| Message ID | 64e08d3a1838ed8507f33fae895545372960522f.camel@gmail.com |
|---|---|
| Headers | show |
| Series | guix: node-build-system: Support compiling add-ons with node-gyp. | expand |
Hi Liliana, Thanks for putting this together! I’m starting to think that we might actually land this series pretty soon. :) Liliana Marie Prikler <liliana.prikler@gmail.com> writes: > 4. Regexps :) I doubt regex support will be broadly useful here. Putting the anchors in every package name (e.g., "^tap$") makes for a lot of noise. My (wild) guess would be that regexes will save us listing two dependencies for one out of every ten Node packages. Given that, my preference would be to not bother with regex support. You wrote this in another message: > I think it'd be beneficial if delete-dependencies could delete > dependencies based on their name matching a regexp rather than a > string exactly. This would make some of your lists shorter > (e.g. "karma.*"), but there might be a debate on whether to use > "^karma.*$" or whether to only consider regexps that match the > dependency fully. If nothing else, I’m certainly on the other side of this debate! :) If every string is going to be treated as a pattern, we should have it match fully by default. That is, the anchors should be implicit. For the very rare (never?) case where you want to avoid anything that so much as has “foo” in the name, it’s pretty easy to write ".*foo.*". -- Tim
Hi, Am Freitag, dem 07.01.2022 um 11:49 -0500 schrieb Timothy Sample: > Liliana Marie Prikler <liliana.prikler@gmail.com> writes: > > 4. Regexps :) > > I doubt regex support will be broadly useful here. Putting the > anchors in every package name (e.g., "^tap$") makes for a lot of > noise. My (wild) guess would be that regexes will save us listing > two dependencies for one out of every ten Node packages. Given that, > my preference would be to not bother with regex support. My reason to include them is that we can already see a number of packages requiring typescript(.*) for a number of (.*) -- similarly karma and mocha -- in the patch set given by Philip. I do think regexps will be less useful later on and could very well become obsolete by the time we have a full bootstrap of everything, but we don't have an ETA on that, so for now I'd like to have that capability. I see two potential solutions here. First is matching the whole string as you suggested and discussed below. Second is opting in to regexp in general (there are some ".js" things, that would otherwise require escaping). This would take the form of the user writing '("foo" "bar" (regexp "^baz$")) as UNWANTED, and it'd be interpreted as the predicates (equal? S "foo") ; alternatively string=? (equal? S "bar") (string-match S "^baz$") WDYT? > > I think it'd be beneficial if delete-dependencies could delete > > dependencies based on their name matching a regexp rather than a > > string exactly. This would make some of your lists shorter > > (e.g. "karma.*"), but there might be a debate on whether to use > > "^karma.*$" or whether to only consider regexps that match the > > dependency fully. > > If nothing else, I’m certainly on the other side of this debate! :) > If every string is going to be treated as a pattern, we should have > it match fully by default. That is, the anchors should be implicit. > For the very rare (never?) case where you want to avoid anything that > so much as has “foo” in the name, it’s pretty easy to write > ".*foo.*". Full string matches are my preference too, but I only found the function that does partial match. Is there an easier solution than checking whether the matched string equals the input to string-match? Cheers
Liliana Marie Prikler <liliana.prikler@gmail.com> writes: > Hi, > > Am Freitag, dem 07.01.2022 um 11:49 -0500 schrieb Timothy Sample: >> Liliana Marie Prikler <liliana.prikler@gmail.com> writes: >> > 4. Regexps :) >> >> I doubt regex support will be broadly useful here. Putting the >> anchors in every package name (e.g., "^tap$") makes for a lot of >> noise. My (wild) guess would be that regexes will save us listing >> two dependencies for one out of every ten Node packages. Given that, >> my preference would be to not bother with regex support. > My reason to include them is that we can already see a number of > packages requiring typescript(.*) for a number of (.*) -- similarly > karma and mocha -- in the patch set given by Philip. I do think > regexps will be less useful later on and could very well become > obsolete by the time we have a full bootstrap of everything, but we > don't have an ETA on that, so for now I'd like to have that capability. > > I see two potential solutions here. First is matching the whole string > as you suggested and discussed below. Second is opting in to regexp in > general (there are some ".js" things, that would otherwise require > escaping). This would take the form of the user writing > '("foo" "bar" (regexp "^baz$")) as UNWANTED, and it'd be interpreted as > the predicates > (equal? S "foo") ; alternatively string=? > (equal? S "bar") > (string-match S "^baz$") > WDYT? Or option three, which is the simplest one; no regex (regexen?), just a big bag of boring strings. To be honest, the current set of packages don't convince me of a need for such a facility yet. >> > I think it'd be beneficial if delete-dependencies could delete >> > dependencies based on their name matching a regexp rather than a >> > string exactly. This would make some of your lists shorter >> > (e.g. "karma.*"), but there might be a debate on whether to use >> > "^karma.*$" or whether to only consider regexps that match the >> > dependency fully. >> >> If nothing else, I’m certainly on the other side of this debate! :) >> If every string is going to be treated as a pattern, we should have >> it match fully by default. That is, the anchors should be implicit. >> For the very rare (never?) case where you want to avoid anything that >> so much as has “foo” in the name, it’s pretty easy to write >> ".*foo.*". > Full string matches are my preference too, but I only found the > function that does partial match. Is there an easier solution than > checking whether the matched string equals the input to string-match? > To put it another way: a package that has to delete about 50 dependencies before it can be packaged in guix proper is allowed to look ugly! It can serve as a very unambigous list of things to possibly improve in a package, while being easy to review as well if changes are later proposed (e.g., you see 'node-karma-runner' being added to inputs, as well as the "karma-runner" string being removed from the list of dependencies to patch out). With regex you always need to hunt down if what is actually happening is what was intended to happen. In addition, since the things-that-can-be-matched by the regex w.r.t. a particular package.json file are already known and clearly enumerated, the only advantage regex brings us here is brevity. The biggest advantage of not using regex is that after a later package update, you can't inadvertently patch out a dependency that was added by upstream. Before the DRY brigade comes to take my guix REPL, I'd argue that any duplication here is incidental. What happens if we want to patch out "karma" and "karma-chrome-launcher", but for some reason want to keep "karma-browserify"?[1] Either way, the reviewer has to do a double take to see whether a change from "karma.*" to the two specifications "karma-chrome-launcher" and "karma" actually gives the expected output w.r.t. the package.json file that is being patched. I appreciate that this patch series is being dragged to the finish line and give my gratitude to everyone involved. - Jelle [1] I know this particular example is ridiculous, but the fact that you have to know what these karma things are and how they relate to eachother to be able to properly review this makes never getting into such a situation all the more worth it to me.
Hi, I'm excited to take a look at this! On 1/7/22 14:43, Liliana Marie Prikler wrote: > Hi, > > Am Freitag, dem 07.01.2022 um 11:49 -0500 schrieb Timothy Sample: >> Liliana Marie Prikler <liliana.prikler@gmail.com> writes: >>> 4. Regexps :) >> >> I doubt regex support will be broadly useful here. Putting the >> anchors in every package name (e.g., "^tap$") makes for a lot of >> noise. My (wild) guess would be that regexes will save us listing >> two dependencies for one out of every ten Node packages. Given that, >> my preference would be to not bother with regex support. > My reason to include them is that we can already see a number of > packages requiring typescript(.*) for a number of (.*) -- similarly > karma and mocha -- in the patch set given by Philip. I do think > regexps will be less useful later on and could very well become > obsolete by the time we have a full bootstrap of everything, but we > don't have an ETA on that, so for now I'd like to have that capability. I think I agree with both of you, with all of the countervailing considerations. My base position is that regexps should not be mandatory or default. It was very convenient to be able to just copy--paste package names from "package.json" into '#:absent-dependencies' (or whatever). I can imagine it being useful for tooling, too, to be able to find just from the package definition the dependencies which are being deleted, rather than having to either download the origins or try to reconstruct names from regexps. But I do know that there are some families of Node.js tools we don't have packaged that come as a bunch of related "devDependencies", and I can see the convenience of being able to remove, say, "@types/.*", "eslint-.*", and ".*-webpack-plugin" succinctly. Ultimately, I agree with Liliana that the right solution to those missing tools is to package them for Guix! My experience so far with workarounds like esbuild is that they are very useful for bootstrapping and pragmatically producing usable packages, but that trying to replace the build tooling of the entire Node.js universe by hand is not a viable solution. (To be slightly more specific, I've encountered some problems with conflicting expectations about whether to consume and/or produce ES6 vs CommonJS modules, for packages that are more like "libraries" than "applications".) I think it would be fine to remove the regexp support, at least for now. Alternatively, I would also be ok with ... > > I see two potential solutions here. First is matching the whole string > as you suggested and discussed below. Second is opting in to regexp in > general (there are some ".js" things, that would otherwise require > escaping). This would take the form of the user writing > '("foo" "bar" (regexp "^baz$")) as UNWANTED, and it'd be interpreted as > the predicates > (equal? S "foo") ; alternatively string=? > (equal? S "bar") > (string-match S "^baz$") > WDYT? the second proposal, or a slight variant on it in which the user would write: `("foo" "bar" ,(make-regexp "baz")) Since this (either variant) would not change the interpretation of strings in the list of dependencies to delete, it could also be added later without breaking compatibility. > >>> I think it'd be beneficial if delete-dependencies could delete >>> dependencies based on their name matching a regexp rather than a >>> string exactly. This would make some of your lists shorter >>> (e.g. "karma.*"), but there might be a debate on whether to use >>> "^karma.*$" or whether to only consider regexps that match the >>> dependency fully. >> >> If nothing else, I’m certainly on the other side of this debate! :) >> If every string is going to be treated as a pattern, we should have >> it match fully by default. That is, the anchors should be implicit. >> For the very rare (never?) case where you want to avoid anything that >> so much as has “foo” in the name, it’s pretty easy to write >> ".*foo.*". > Full string matches are my preference too, but I only found the > function that does partial match. Is there an easier solution than > checking whether the matched string equals the input to string-match? I also think full string matches are better, regardless of any of the above. Here's an implementation that avoids unneeded copies and string comparison: (define (regexp-match-exact? rx str) (define m (regexp-exec rx str)) (and m (= (match:start m) 0) (= (match:end m) (string-length str)))) (This should also avoid repeatedly compiling the regexp, as 'string-match' would do.) More soon, I hope! -Philip
Hi, On 1/6/22 12:45, Liliana Marie Prikler wrote: > Hi Philip, > > Please pardon the large flood of messages in a short enough of time > without waiting for a reply. I tried my best to come up with a v8 that > addresses my personal concerns quickly. > > My main changes were to patches 3-5, but note how deleting dependencies > became significantly easier in 6pp. To list the main differences: > 1. I've implemented the alist/JSON utilities in terms of SRFI-1 and SRFI- > 2. I shortened the additional JSON API to jsobject-ref and jsobject-update*. > I still believe alist->json-object and json-object->alist would be more > useful (in particular, the main operation of jsobject-union is actually > alist-flatten), but I digress. While some of these changes are not to my taste, there's nothing I can't live with for the sake of getting this patch series applied, finally. > 3. I made it so that delete-dependencies also acts on peerDependencies. > That way, we don't have to dance around ordering. I think this is not quite correct. (Actually, I suspect more broadly that node-build-system's handling of peerDependencies is not quite correct, but wrapping my head around the semantics of peerDependencies is on my to-do list for after these patches are applied. Here's one thing I want to read and understand: https://pnpm.io/how-peers-are-resolved) NPM does not try to install packages in "peerDependencis" during 'npm install' (out 'configure' phase). The problem arises because because our 'patch-dependencies' phase adds the "peerDependencies" as additional "dependencies". (Why? I don't fully understand, but I guess because it wants them to be installed.) We want absent "peerDependencies" to not be listed in "dependencies", but I don't think we want to delete them from "peerDependencies": at a minimum, we do not need to, and it seems like it might cause problems that I don't fully understand. (This is one of the reasons I preferred to handle absent dependencies in the 'patch-dependencies' phase.) > 4. Regexps :) Hopefully addressed in my previous email :) Jelle makes good arguments for the no-regexps side. I'm genuinely on the fence, which suggests to me the best course might be to leave it as a possible future extension (as we're doing with '#:absent-dependencies'). > PS: If someone else wants to push these in my absence, please adjust the > signoffs. Also, if those mail headers are broken, don't forget to reset > Philip as author. For the patches where you've made substantive changes to the implementation or the commit message, maybe there should be more of an annotation than just "Signed-off-by". I'm not super familiar with the Git etiquette here, but one suggestion I've seen is something like: Signed-off-by: Random J Developer <random@developer.example.org> [lucky@maintainer.example.org: struct foo moved from foo.c to foo.h] Signed-off-by: Lucky K Maintainer <lucky@maintainer.example.org> It's not a big deal, I could just imagine getting confused some months or years from now about what exactly I wrote or didn't write. (TBH the numerous versions of this series are already a bit unwieldy.) Time permitting, I'll send some more comments, but the only things I think need to be addressed before merging are peerDependencies and regexps. Thanks for helping to keep things moving! -Philip
Am Freitag, dem 07.01.2022 um 22:02 +0100 schrieb Jelle Licht: > To put it another way: a package that has to delete about 50 > dependencies before it can be packaged in guix proper is allowed to > look ugly! It can serve as a very unambigous list of things to > possibly improve in a package, while being easy to review as well if > changes are later proposed (e.g., you see 'node-karma-runner' being > added to inputs, as well as the "karma-runner" string being removed > from the list of dependencies to patch out). With regex you always > need to hunt down if what is actually happening is what was intended > to happen. > > In addition, since the things-that-can-be-matched by the regex w.r.t. > a particular package.json file are already known and clearly > enumerated, the only advantage regex brings us here is brevity. The > biggest advantage of not using regex is that after a later package > update, you can't inadvertently patch out a dependency that was added > by upstream. I'm not sure this is a good argument. Even with a huge ass list (put the hyphen where you want to), upstreams can strengthen and weaken coupling with an update, both incidentally and purposefully. So let's say we decided to delete node-karma for version 0.2.0 of a package and that worked fine until 0.6.5, but it's a requirement in 0.7.0 -- not that we'd run tests or anything, because those require tap -- I don't think either form is particularly helpful and in fact, I'd urge reviewers to take a close look at the package.json before and after regardless form. > Before the DRY brigade comes to take my guix REPL, I'd argue that any > duplication here is incidental. What happens if we want to patch out > "karma" and "karma-chrome-launcher", but for some reason want to keep > "karma-browserify"?[1] Either way, the reviewer has to do a double > take to see whether a change from "karma.*" to the two specifications > "karma-chrome-launcher" and "karma" actually gives the expected > output w.r.t. the package.json file that is being patched. I think node packages do generally follow a pattern here, but let's assume they don't and your example works that way. In that case, I'd argue to make that regexp "karma(-browserify)?". And if later on karma-chrome-launcher is to be added to that list, but karma-icecat is allowed, "karma(-(browserify|chrome-launcher))?" In other words, we can as a community discourage the usage of ".*" without condemning all regexp. Now I'm personally not convinced that disallowing "karma.*" altogether is useful if we don't even have a karma package to go with -- and I'm very sure we'd notice karma being packaged and check our karma dropping packages -- but I'm willing to accept de gustibus here. Cheers
Hi, Am Freitag, dem 07.01.2022 um 16:07 -0500 schrieb Philip McGrath: > My base position is that regexps should not be mandatory or default. > It was very convenient to be able to just copy--paste package names > from "package.json" into '#:absent-dependencies' (or whatever). I can > imagine it being useful for tooling, too, to be able to find just > from the package definition the dependencies which are being deleted, > rather than having to either download the origins or try to > reconstruct names from regexps. I don't think tooling should be too big of an issue, since you could teach your tool to support regexp. I.e. instead of checking whether a newly added package has the name, you check whether it fits the pattern. That might generate false positive in some cases, perhaps even many, but we can (and should) refine regexps that are too generic. > Alternatively, I would also be ok with [...] a slight variant on it > in which the user would write: > > `("foo" "bar" ,(make-regexp "baz")) > > Since this (either variant) would not change the interpretation of > strings in the list of dependencies to delete, it could also be added > later without breaking compatibility. That's true, but the main reason to add it from my perspective is that we don't need to change how regexps are interpreted from the rest of Guix to make basic strings match fully. For the fun of it, we could also extend this to procedures of a single value so that one can finally supply (const #t) :D > I also think full string matches are better, regardless of any of the > above. Here's an implementation that avoids unneeded copies and > string comparison: > > (define (regexp-match-exact? rx str) > (define m (regexp-exec rx str)) > (and m > (= (match:start m) 0) > (= (match:end m) (string-length str)))) I can't say this is more beautiful than (equal? (and=> (regexp-exec rx str) match:substring) str), but there's only that many ways of making a partial match a full one and Guile core has none of them...
Liliana Marie Prikler <liliana.prikler@gmail.com> writes: > Am Freitag, dem 07.01.2022 um 22:02 +0100 schrieb Jelle Licht: >> To put it another way: a package that has to delete about 50 >> dependencies before it can be packaged in guix proper is allowed to >> look ugly! It can serve as a very unambigous list of things to >> possibly improve in a package, while being easy to review as well if >> changes are later proposed (e.g., you see 'node-karma-runner' being >> added to inputs, as well as the "karma-runner" string being removed >> from the list of dependencies to patch out). With regex you always >> need to hunt down if what is actually happening is what was intended >> to happen. >> >> In addition, since the things-that-can-be-matched by the regex w.r.t. >> a particular package.json file are already known and clearly >> enumerated, the only advantage regex brings us here is brevity. The >> biggest advantage of not using regex is that after a later package >> update, you can't inadvertently patch out a dependency that was added >> by upstream. > I'm not sure this is a good argument. Even with a huge ass list (put > the hyphen where you want to), upstreams can strengthen and weaken > coupling with an update, both incidentally and purposefully. So let's > say we decided to delete node-karma for version 0.2.0 of a package and > that worked fine until 0.6.5, but it's a requirement in 0.7.0 -- not > that we'd run tests or anything, because those require tap -- I don't > think either form is particularly helpful and in fact, I'd urge > reviewers to take a close look at the package.json before and after > regardless form. Rather the other way around; if we do not allow regex, we will see that there is a new dependency that we are currently neither patching nor adding to the inputs. We should celebrate any accurate build time failures we can make happen! >> Before the DRY brigade comes to take my guix REPL, I'd argue that any >> duplication here is incidental. What happens if we want to patch out >> "karma" and "karma-chrome-launcher", but for some reason want to keep >> "karma-browserify"?[1] Either way, the reviewer has to do a double >> take to see whether a change from "karma.*" to the two specifications >> "karma-chrome-launcher" and "karma" actually gives the expected >> output w.r.t. the package.json file that is being patched. > I think node packages do generally follow a pattern here, but let's > assume they don't and your example works that way. In that case, I'd > argue to make that regexp "karma(-browserify)?". And if later on > karma-chrome-launcher is to be added to that list, but karma-icecat is > allowed, "karma(-(browserify|chrome-launcher))?" In other words, we ^ I would object to this (constructed example) in a patch review, fwiw. At this stage, we expect the author to determine which dependencies to leave in and out, then correctly encode these choices as a regex, which is then decoded again by a reviewer who then must manually verify that this decoded intent makes sense with regards to both the listed inputs and the actual unpatched contents of package.json. Without regex, there's just a few details that need to be kept in mind for each step, for both author and reviewer(s). Adding something to the deleted dependencies can either: - fix a (newly) broken build - remove an optional dependency, and can most likely be removed from existing inputs. Removing something from the deleted dependencies can either: - be a no-op: so the existing deleted dependency was (made) redundant - enable an optional dependency, which should most likely be added to the inputs. That's it! Some creative types can put this in fancy decision graph and we have our "You Too Can Review Node Packages" campaign! This all just to clarify my position, but perhaps also demonstrate that it's a subjective preference, so 'agree to disagree' is a fine position to take here. > can as a community discourage the usage of ".*" without condemning all > regexp. > > Now I'm personally not convinced that disallowing "karma.*" altogether > is useful if we don't even have a karma package to go with -- and I'm > very sure we'd notice karma being packaged and check our karma dropping > packages -- but I'm willing to accept de gustibus here. This is a very pragmatic example indeed! If some other folks could still weigh in, I'd be okay with the resulting decision either way. - Jelle
Hi, Am Freitag, dem 07.01.2022 um 17:11 -0500 schrieb Philip McGrath: > I think this is not quite correct. > > (Actually, I suspect more broadly that node-build-system's handling > of peerDependencies is not quite correct, but wrapping my head around > the semantics of peerDependencies is on my to-do list for after these > patches are applied. Here's one thing I want to read and understand: > https://pnpm.io/how-peers-are-resolved) > > NPM does not try to install packages in "peerDependencis" during 'npm > install' (out 'configure' phase). The problem arises because because > our 'patch-dependencies' phase adds the "peerDependencies" as > additional "dependencies". (Why? I don't fully understand, but I > guess because it wants them to be installed.) We want absent > "peerDependencies" to not be listed in "dependencies", but I don't > think we want to delete them from "peerDependencies": at a minimum, > we do not need to, and it seems like it might cause problems that I > don't fully understand. > > (This is one of the reasons I preferred to handle absent dependencies > in the 'patch-dependencies' phase.) I'd like to be able to understand that too, but npm still boggles my mind. I think node-build-system's implementation is a rather pragmatic one; it forces you to use just a single combination of versions for all of those rather than relying on node trickery (on a related note, perhaps we ought to make inputs in node-build-system propagated-inputs to be on the safe side). > > 4. Regexps :) > > Hopefully addressed in my previous email :) Jelle makes good > arguments for the no-regexps side. I'm genuinely on the fence, which > suggests to me the best course might be to leave it as a possible > future extension (as we're doing with '#:absent-dependencies'). We do already have threads on the regexp thing, so I'm not going to respond here to keep it manageable. The change is a rather small one inside node-build-system itself, but you have to expand those strings again ;) > For the patches where you've made substantive changes to the > implementation or the commit message, maybe there should be more of > an annotation than just "Signed-off-by". I'm not super familiar with > the Git etiquette here, but one suggestion I've seen is something > like: > > Signed-off-by: Random J Developer <random@developer.example.org> > [lucky@maintainer.example.org: struct foo moved from foo.c to > foo.h] > Signed-off-by: Lucky K Maintainer <lucky@maintainer.example.org> > > It's not a big deal, I could just imagine getting confused some > months or years from now about what exactly I wrote or didn't write. > (TBH the numerous versions of this series are already a bit > unwieldy.) If you don't like my phrasing in a particular message, you are free to change it for v9 (or if you don't want a v9 to be reviewed, just reply to the individual one with how you'd reword it and I'll try to keep my changes to your version minimal). I simply tried to adapt your messages to the generally accepted style, which means moving long-form discussions out of the ChangeLog into the message body and correcting every variable to variable. Now I do admit that I used some more flowery parts for 3-5 and that's completely on me, but 3 already has me as Co-Author anyway. > Time permitting, I'll send some more comments, but the only things I > think need to be addressed before merging are peerDependencies and > regexps. Cool. Let's just not forget to send a v9 once we have what looks like to be a reasonable action (assuming it's not a do-nothing, in which case I just need to reword some of your commit messages again). In my personal experience, patches help a stalled discussion tremendously. Cheers
Hi Jelle, Am Samstag, dem 08.01.2022 um 00:07 +0100 schrieb Jelle Licht: > Liliana Marie Prikler <liliana.prikler@gmail.com> writes: > > [...] > > I don't think either form is particularly helpful and in fact, I'd > > urge reviewers to take a close look at the package.json before and > > after regardless form. > > Rather the other way around; if we do not allow regex, we will see > that there is a new dependency that we are currently neither patching > nor adding to the inputs. We should celebrate any accurate build time > failures we can make happen! We're arguing past each other. What I'm saying is that with neither you can assure your exclude list is actually reasonable in any sense of the word. > > > At this stage, we expect the author to determine which dependencies > to leave in and out, then correctly encode these choices as a regex, > which is then decoded again by a reviewer who then must manually > verify that this decoded intent makes sense with regards to both the > listed inputs and the actual unpatched contents of package.json. Again de gustibus, but I'd rather read one very well thought out "typescript.*" than dozens of lines excluding typescript-something. Also, with longer/well-known prefixes, it typically becomes easier to reason about a single regexp rather than seeing again 3+ lines of any given package family and missing the forest for the trees. > Without regex, there's just a few details that need to be kept in > mind for each step, for both author and reviewer(s). > > Adding something to the deleted dependencies can either: > - fix a (newly) broken build > - remove an optional dependency, and can most likely be removed from > existing inputs. > > Removing something from the deleted dependencies can either: > - be a no-op: so the existing deleted dependency was (made) redundant > - enable an optional dependency, which should most likely be added to > the inputs. > > That's it! Some creative types can put this in fancy decision graph > and we have our "You Too Can Review Node Packages" campaign! That's not at all the complete list of possibilities for node packages. Unless you define "optional" in the loosest sense, meaning "yeah, you can build it, but try calling a simple function and you'll get an import error of doom", which is possible because node doesn't statically verify zilch (*). That's the main reason Philip rejected my "just restrict ourselves to what we find in inputs" suggestion. Cheers > > (*) double negative meant as actual negative
Hi, On 1/7/22 18:47, Liliana Marie Prikler wrote: > Hi, > > Am Freitag, dem 07.01.2022 um 17:11 -0500 schrieb Philip McGrath: >> I think this is not quite correct. >> >> (Actually, I suspect more broadly that node-build-system's handling >> of peerDependencies is not quite correct, but wrapping my head around >> the semantics of peerDependencies is on my to-do list for after these >> patches are applied. Here's one thing I want to read and understand: >> https://pnpm.io/how-peers-are-resolved) >> >> NPM does not try to install packages in "peerDependencis" during 'npm >> install' (out 'configure' phase). The problem arises because because >> our 'patch-dependencies' phase adds the "peerDependencies" as >> additional "dependencies". (Why? I don't fully understand, but I >> guess because it wants them to be installed.) We want absent >> "peerDependencies" to not be listed in "dependencies", but I don't >> think we want to delete them from "peerDependencies": at a minimum, >> we do not need to, and it seems like it might cause problems that I >> don't fully understand. >> >> (This is one of the reasons I preferred to handle absent dependencies >> in the 'patch-dependencies' phase.) > I'd like to be able to understand that too, but npm still boggles my > mind. I mean, I did start writing this patch series because I find it more understandable than just using npm :) > I think node-build-system's implementation is a rather pragmatic > one; it forces you to use just a single combination of versions for all > of those rather than relying on node trickery I will send a v9 that doesn't delete "peerDependencies" and just rely on doing the ordering properly. Hopefully, we can improve the situation later, once we understand how "peerDependencies" are actually supposed to work (and/or adopt '#:absent-dependencies' :) ). I don't know of any concrete problems that would be caused by overzealously deleting "peerDependencies", but I wouldn't know of them, since that's not the behavior I've been testing all this time. > (on a related note, > perhaps we ought to make inputs in node-build-system propagated-inputs > to be on the safe side). I think that might not actually lead Node.js to find all of the modules, and some things I've been reading with interest (but not yet fully understanding) suggests that the opposite, i.e. creating a more strict "node_modules", is actually useful. [1] [2] > >>> 4. Regexps :) >> >> Hopefully addressed in my previous email :) Jelle makes good >> arguments for the no-regexps side. I'm genuinely on the fence, which >> suggests to me the best course might be to leave it as a possible >> future extension (as we're doing with '#:absent-dependencies'). > We do already have threads on the regexp thing, so I'm not going to > respond here to keep it manageable. The change is a rather small one > inside node-build-system itself, but you have to expand those strings > again ;) I am not 100% clear---if I'm wrong, please speak up!---but my sense from the previous thread is that: 1. some people have reservations about some regexp proposals; 2. everyone who has liked any of the regexp proposals has been ok with one of the options for distinguishing regexps from non-regexp strings, either '(regexp "foo") or (make-regexp "foo"), with another dimension being whether or not to require full matches; and 3. with either of the options from #2---as long as regexps are using some representation that answers #f to 'string?'---regexp support could be added as a fully compatible future extension. So I think---I hope!---a version without regex support could get everyone's assent. Unless someone tells me otherwise first, that's what I'll do in v9. >> Time permitting, I'll send some more comments, but the only things I >> think need to be addressed before merging are peerDependencies and >> regexps. > Cool. Let's just not forget to send a v9 once we have what looks like > to be a reasonable action (assuming it's not a do-nothing, in which > case I just need to reword some of your commit messages again). In my > personal experience, patches help a stalled discussion tremendously. Ok! -Philip [1]: https://pnpm.io/blog/2020/05/27/flat-node-modules-is-not-the-only-way [2]: https://medium.com/pnpm/pnpms-strictness-helps-to-avoid-silly-bugs-9a15fb306308