-
-
Notifications
You must be signed in to change notification settings - Fork 910
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix issue on import of legacy dependent packages #584
Conversation
see https://stackoverflow.com/a/69564152/589493 this only fixes the issue for v1, additional patches needed for the other versions not sure this is a desired "fix" as i am not sure this is considered a bug itfp by maintainers.
Deep requires have intentionally been deprecated in v7.x and removed v8.x, upgrade instructions are given in the CHANGELOG, more context in #361 (comment). Why do you want to re-introduce deep requires? |
not sure if you saw the link to SO post in the description. this seems to be an issue for a number of people in different setups. i can only explain what mine is: i am not importing/requiring the module directly in our src. uuid is being used by a jest dependency in our case afaict. the problem comes up when bundling with webpack. our project is bundled by webpack to a bin for node runtime. unfortunately it seems to bundle this part of the dev deps, which i agree is another root cause to solve this as well here. OTOH it seemed to me that generally moving away from "deep requires" in your dist breaks loading other consuming packages (maybe only in certain module-loading i.e bundling modes). i guess that these dependent consumers of uuid just got the latest dist as they use a ^ version in their deps? which is one explanation why this issue suddenly came up updating our deps probably? |
@tchakabam Jest itself doesn't use uuid and doesn't have it as transitive dependency. |
@TrySound @ctavan Jest-reporter is a sub-package of jest that does use uuid. And also other packages in our tooling (webpack worker-loader) seem to be using it and aren't able to use the novel import mode it seems. i have to admit, rather than looking in detail into this issue and understanding it in depth, I am just trying to find workarounds or fix compatibility somhow. Because it isn't my project directly as you may understand :) |
Here's the whole jest/reporters dependencies tree. There is no uuid there. Webpack and worker-loader do not use it as well. Please look in your lock file for dependency which uses uuid and list here. |
Or you can put here your lockfile (in gist please) |
@TrySound For future reference, npmgraph.js.org is way better for inspecting dependency graphs. (* cough * Disclaimer * cough * ) |
Ok, apologies I agree my report may be confusing and not accurate in the analysis. I am still trying to figure this out. Will get back to you with some data asap. Thanks for your help so far! |
anyhow seems i am not the only one with this issue, see #594 |
Probably we were not using the latest version of all this stuff, and thus what you were looking at was not my depedencies. I am completely sure i saw from the lockfile jest-reporter was using uuid package. But I am wondering what can be your concept here at all? Do you expect all dependents to change their import mode (and also all the user projects with different bundling tools/modes) as you people released a new version that just breaks the previous distro/import mode? Why dont you just keep backward compatible? |
Because it doesn't break anything if uuid has version with
Maybe it's some very old version with incorrectly specified version. Latest jest version doesn't have this problem so better upgrade. |
I get your point :) Meanwhile, that´s in theory ;) So, I cant provide our plain package-lock file for project disclosure reasons, but here is the bit where Hope that helps! thanks anyhow.
|
Ok will try to confirm this asap |
that bit is also interesting. seems that now that we have enforced installing uuid 7.0.3 in the top level, the node-notifier is relying on its sub node_modules install rather than the top-level one. and that somehow then works all well. honestly this is quite a cluster and i cant really dive in too deep in this atm, sorry. i am not claiming to know what the issue is, or that it is necessarily to be fixed on uuid project side. just saw what i saw and tried to find a solution here somehow :) thanks so far!! |
The whole purpose of Semantic Versioning is to provide library authors with a way for safely introducing breaking changes, and developers with a safe way of dealing with such changes by specifying appropriate dependency version ranges. If we start supporting legacy forever just because some other library or application specifies dependencies in a wrong way it would defeat the original purpose of Semantic Versioning. We will therefore not start fixing other people's code by re-introducing technical debt. |
see #594 (comment) |
@ctavan just wanted to help you with providing data as it was requested before. you're welcome ... |
node-notifier uses uuid correctly. See https://unpkg.com/browse/node-notifier@8.0.2/notifiers/toaster.js Maybe it's the issue with npm which install wrong dependencies. Anyway many packages change their api in major releases. "Fixing" uuid doesn't change anything if npm installs packages incorrectly. Just something else will break. |
see https://stackoverflow.com/a/69564152/589493
this only fixes the issue for v1, additional patches needed for the other versions
not sure this is a desired "fix" as i am not sure this is considered a bug itfp by maintainers.