How to handle "bad" packages that require() things that aren't in their package.json? #949
Comments
nickpape
changed the title
nickpape
changed the title
|
There is a way, you can use installation hooks. With an installation hook you can basically override jest's manifest and add the missing dependency. This use case was one of the reasons hooks were added to pnpm. You can read more about it in: Why package managers need hook systems In your case the module.exports = {
hooks: {
readPackage
}
}
function readPackage (pkg) {
if (pkg.name === 'jest-runtime') {
pkg.dependencies = {
slash: '^1.0.0'
}
}
return pkg
} |
|
Thanks @zkochan . Is there any thought of adding a configuration file to handle this? In general, we have been trying to avoid solutions to problems which involve writing custom code - there is more room for confusion and bugs, whereas configuration files can be easily documented and validated. I'd be willing to put together a PR for this if you are interested. For now, I think we add a feature to |
|
We actually planned to implement this as a config (see #651) but then we decided that a js file is a more powerful solution (see #861). With the @vjpr and @andreineculau, do you have any suggestion regarding this? |
Definitely good to have, but I agree that a config option would be easier. Static pjson config is easier to modify with scripts. Use case: Say you have multiple projects (Lerna monorepo for example) that each use a dep that has a missing dep. The easiest thing would be to write a script to go through and add to I'm all for config. We should support Yarn's
I'm not sure if it handles "missing" deps, but if not, we could also add |
|
Well, I wouldn't like to deprecate pnpmfile.js because it can do a lot more
than yarn's `resolutions`.
We can add a plugin that will convert yarn's resolutions to pnpm's
`readPackage()` hook. The plugin would have to be required in the
`pnpmfile.js`.
This solution would keep pnpmfile.js the "the single place of truth".
…On Tue, Dec 5, 2017, 12:17 Vaughan Rouesnel ***@***.***> wrote:
With the pnpmfile.js you can create a hook and publish it as a package
Definitely good to have, but I agree that a config option would be easier.
Static pjson config is easier to modify with scripts.
Use case: Say you have multiple projects (Lerna monorepo for example) that
each need a custom dep. The easiest thing would be to write a script to go
through and add to resolutions key in pjson of each package. Now, the
current pnpmfile.js approach of creating and publishing a package
provides flexibility, but there is so much more friction involved in
publishing a package, than changing a config setting. Also, its not easy to
add this config setting to multiple packages with a script.
I'm all for config.
We should support Yarn's resolutions:
-
https://github.com/yarnpkg/rfcs/blob/master/implemented/0000-selective-versions-resolutions.md#resolutions
- https://yarnpkg.com/en/docs/package-json#toc-resolutions
I'm not sure if it handles "missing" deps, but if not, we could also add
additionalResolutions.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#949 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AB1pm-UliDEwRnZFYjHT4JthOlBUbFjqks5s9Rg_gaJpZM4QxQEP>
.
|
@zkochan I like it. |
So if I manage 20 Git repositories, I would need to add this converter code to pnpmfile.js for each repository, right? When we fix a bug in the converter, we would need to somehow propagate the fix to all our repositories, to keep them consistent. When we do this, if we find that someone else has added their own logic to pnpmfile.js, we would need to merge our update with their customizations. The approach is certainly very flexible, but flexibility has a downside that it makes things more fragmented and complex across your ecosystem. If 95% of people's problems can be solved with a simple, inflexible config file, that can actually be a superior design. It's still great to provide script hooks for the 5% of advanced problems, but I'm not sure if that flexibility should be imposed on the other 95% of people. I suspect this might be why Yarn opted for the config file approach. Just something to think about. |
|
Maybe I am misunderstanding something about how pnpm plugins work. How do they get installed after you do "git clone"? My group manages repos where hundreds of different developers will come and make changes from time to time, often without much familiarity of how the system is set up. If their machine isn't configured right, or if they encounter an error message that isn't understandable, they open a ticket and we have to spend time helping them. Often they get very frustrated, because they are in a hurry to get their fix merged. So we have a strong incentive to make everything as intuitive and consistent as possible. |
I don't know yet, we don't have this plugins yet. The simplest solution might be to have the plugin in pnpm. In that case the const createResolutionsHook = require('pnpm').createResolutionsHook
const resolutions = require('package').resolutions
const readPackage = createResolutionsHook(resolutions)
module.exports = { hooks: { readPackage } }I understand that this is not scalable. Maybe the compromise would be that: if there is no If there is a |
|
I tend to agree with @pgonzal regarding config over plugins. The "bad" package issue is something I run into daily, and it will be like this for the foreseeable future I expect - which is why we should support resolutions as a first-class config option. I've been trying to think of what plugins I would use in the For anything else I would use a separate tool, say like monkey-patching a module to fix its webpack resolving code (https://github.com/ds300/patch-package). |
|
what can you use
|
|
@zkochan Are there examples of these anywhere in the docs/codebase? Would be cool to have some "pnpmfile.js recipes" if not. |
|
not right now. I think adding them to the readme would be good. Or a separate page, with examples |
Reading this is confusing alone :) Many configuration files have strict syntax which prohibits documentation. With that out of the way, validation may be hard as well, even if the serialization standard is popular e.g. json/yaml because the semantics are not validated. This means that you're just as good validating code. Eslint switched to code. Babel switched to code. And the examples go beyond the javascript ecosystem.
It doesn't need to be installed to begin with. npm, pnpm, etc are package managers, they are not build tools. And if you don't have build tools e.g. A suggestion would be to have some-node-module-with-pnpm-hooks available as a git submodule, but even then you have a manual specification if you don't have a build tool e.g. use I do not follow anything that you wrote. Sorry. I'm being honest, not disrespectful. I stand firmly that pnpmfile.js is both as easy and much more powerful than pnpmfile.json . And I do not follow what does publishing packages have to do with it. I must be missing something. Don't you need to publish a config file then to all your 20 git repositories. @pgonzal @vjpr It sounds to me like you're not advocating a config (static) vs hooks-in-a-javascript-file ( |
I was commenting on the idea of using npm published packages for commonly broken deps. E.g. Having a module called This sounds nice, but I'd prefer adding to |
|
-1 for global IMHO the obvious solution is a Why can't we just have both working side-by-side? So anything added to resolutions would be as if you added a hook. |
|
Funny thing is, I am not even sure the resolutions field would work with pnpm. It is related to the way pnpm installs, it doesn't really care about the path of the package (unless it has peer dependencies) see my comment in another issue: #952 (comment). The global pnpmfile might be not a good idea but it can be powerful. Like it can be a function that gets all the pnpm configs and returns a modified set. In that case you'd be able to apply the hooks only in some directories. Something like: module.exports = (opts) => {
if (!opts.prefix.startsWith('/home/src/') {
return opts;
}
// doing some stuff
}So the global pnpmfile would be used only when |
|
By "global", do you mean external to the Git repo folder? That would not be a very good approach for us. We have worked very hard to make our developer experience completely deterministic. This avoids problems where a person complains about a build error, but when I try it on my laptop, everything works fine for me. Impossible to investigate without access to the person's computer, very annoying! Deterministic builds also make it easy to build an old branch from months ago, without running into conflicts with upgraded tools or environmental changes. Currently we do the following things (via the Rush tool):
This approach also minimizes the amount of setup a person has to do before they can build. In the past, we had to give detailed instructions (install gulp, make sure your NPM is the right version, etc). Today pretty much the only setup step is to add your registry credentials to .npmrc. Everything else is locked down. Even when people use Mac/Linux/Windows OS's, the results are very consistent. From this perspective, it wouldn't make sense to see something like "/home" in a config file. :-) |
|
It doesn't necessarily have to be global. It can be shared. As far as I understand you use one npm cache by monorepo. Whichever way you specify the cache location, you can use the same way to specify the pnpmfile location. It can be in the root of the monorepo, not the "/home" dir. |
Our config files always have an associated JSON schema. You can use a library like ajv to validate a config file against its schema, and this will print informative error messages without any effort. If the config file includes a This is generally a better experience than expecting users to edit a script. Scripts are "powerful" only in the sense that they can do anything imaginable. It's like if I wanted to eat lunch, and the restaurant told me: "Here's our kitchen, here's the fridge, go ahead and cook whatever you want!" That is a very powerful and flexible option. A few people might like that. If I wanted a really unusual lunch, it's a cool offer. But most people prefer a simple restaurant menu with a fixed list of options. Too much flexibility is bad for them. :-)
Not really. In practice each Git repository ends up with its own set of versions (i.e. shrinkwrap file). As long as there aren't tons of "bad" packages, it would be fine to manage them independently. I was envisioning something like a "pnpm.json" config file stored in Git. If I run |
|
We are giving a powerful feature to our users. They can create their own wrappers around it. If you want it to work with json files, go ahead and create a hook that will work with it. As I mentioned above, the pnpmfile should not necessarily be per project or global. It can be per repo, for instance. If someone will come up with a super-intuitive solution, maybe we'll merge it to pnpm. But I would definitely start with the existing stack. Deprecating things is not easy. |
|
That seems pretty reasonable to me. |
|
@vjpr I read stuff like 10 times, and even wrote a full wall of text, until I realized (I think) what you're saying: it is easier to merge some JSON, or add a new line to a file (or n files, because you have n projects that all have a broken dependency) than to modify code in a pnpmfile.js, assuming that some of those n projects do not share the same pnpmfile.js content. If you can confirm that's the case, the technical solution (a raw example, not a UX solution) would be that a variety of pnpmfiles with their own readPackage hook are placed in a folder, and each and every one will be executed. In that case, you'd copy the same file to all n projects. You upgrade all n projects to a version of "foo" that is not broken, then you'd just delete the pnpmfile that fixes foo. If it's not clear, I'm not advocating that everyone should implement such a top-level pnpmfile. Maybe the UX solution could be that pnpm looks for a pnpmdir in the root and queue hooks in alphabetical order of the files in that dir, etc. I don't know. I'm not proposing a system, I'm just highlighting that your problem has a solution today, primarily because the functionality is so flexible. @zkochan would you be so kind and clarify this for me: pnpmfile.js is only read from the folder where pnpm is executed, right? so if a dependency will have a pnpmfile.js of its own, it is ignored? I think we had this discussion before, and I thought it it is not ignored, but you corrected me, and I stand corrected. Just like ".npmrc", settings like these should only be considered for the "ultimate" package.
The reasons you listed are the same for anything. A global/user level gitignore. A global/user level .npmrc . Etc. What we need to remember is that pnpmfile.js is not a dumb mechanism. If you detect that foo@1.2.3 is broken, you can fix only foo@1.2.3 not 1.2.4, not 1.2.2. When foo is upgraded and that version is not broken, then I'd have some leftovers in there that maybe I will always forget to clean, but it doesn't break my development whatsoever. Obviously if I say that I replace foo@* with fixed-foo@1.2.3 then I've got a problem, but that problem is mine alone. The reason for a global and or user level and or a "shared" pnpmfile.js is that I or my company could deny installing GPL packages, for example. The "problem" is that this would require pnpmfile.js to "merge"=add hooks, not override them. But that deserves a discussion on its own.
That cannot possible withstand reality. Maybe you minimize problems, yes. But unless you develop in a VM/container=sandbox environment, there's always something global. That said +1000 for you/your team's efforts in this direction.
I take the liberty to continue: ..., until they are not happy with the limitation and then they demand an ever-growing selection of simple restaurants with an ever-growing fixed list of options. Examples: eslint/eslint#6460 http://babeljs.io/docs/usage/babelrc/#env-option . I can find more examples, but it's late here. Both of those examples though are non-issues when using the programmatic configuration of eslint and babel.
Care enough to elaborate? You still need to commit changes to this pnpm.json in each of the 20 git repos, don't you? PS: this thread is hard to keep in check :) @zkochan 's 2nd comment on this thread #949 (comment) hijacked the thread to begin with because AFAICS that does not address @nickpape-msft 's take on code vs static config file. And then others came to pitch in how they would work with a static config file, etc, but the reasonings brought forward are very specific, and what I conclude is that pnpm would benefit a great deal from some pnpmfile.js examples. If in situation A, try smth like this. If in situation A but also doing B, try smth like this. etc. I'm not demanding/expecting someone to write them, just that they could fill a void. Especially in this situation where like @zkochan mentioned nothing stops anyone from having a "blind" pnpmfile.js driven by a static config file: you decide the format of that config file, and then your pnpmfile.js is coded to read it and reply to readPackage calls as instructed in the config file. There are a couple of ideas, specific scenarios that should not be forgotten, and if anyone involved on this thread has the energy, I'd welcome them to open up new issues with details about their specific environments, their specific hurdle with pnpmfile.js and how they envision an easier alternative. If not for other reasons, that will be a good platform to create pnpmfile.js examples. |
|
I think the way forward with this unwieldy thread is the need for concrete use cases and code examples. Example: Dep has missing dep I open missing dep issues on a weekly basis with 3rd party repos. E.g. The latest was graphile/graphile-engine#119. Usually they are fixed quickly (within 7 days if the repo is active). Now when this happens, what can I do to fix it?
This was install the missing dep
The alternative with the
|
|
Instead of just fixing these missing dependencies in the consuming projects, we should take the effort of submitting a pull request to the project which left out the dependency, so the wider ecosystem can benefit from it. While we were waiting for the PR being reviewed, we just used our fork directly in our instead of (This approach only works for direct dependencies though, I guess, not for transitive ones) |
|
I feel like it would be helpful to have a single list of known packages that have this problem. |
|
@zoltan once said:
This seems like the biggest annoyance when it comes to What if PNPM scanned |
|
I'm strongly in favor of adding |
|
related: #1082 I just read the specs of the "resolutions" field at https://github.com/yarnpkg/rfcs/blob/master/implemented/0000-selective-versions-resolutions.md Issues I see:
It is currently impossible to implement with pnpm. pnpm always uses one dependency set for a version of a package in one node_modules. So if there is Also, For pnpm, something like this would work: "pnpmResolution": {
"foo@^3.0.0": {
"bar": "1.0.0"
}
} |
|
Okay then let's make |
|
We could add a exports.resolutions = {
// Pin the "foo@^3.0.0" dependency to "^2.0.0" for every dependent package in the tree.
'foo@^3.0.0': '^2.0.0',
'foo@^2.0.0': {
// Pin the "bar" dependency of "foo@^2.0.0" only.
'bar': '^1.0.0'
}
} |
|
You could use import-scan (< 1 kB) to parse any |
|
I would rather not add additional complexity just to support dynamic installations of imports in pnpmfile. If someone really needs it, they can preinstall the needed dependencies and commit them to the repo. Or bundle them into pnpmfile.js |
I think that severely harms the usability of Thoughts on this suggestion? |
|
yes, we can support something like that but a lot of people asked for a non-executable way of declaring resolutions, so it will have to be something like a |
pnpm version:
1.23.1Code to reproduce the issue:
Create a
package.jsonwith the following contents:{ "name": "test-app", "version": "1.0.0", "description": "", "main": "index.js", "scripts": { "test": "echo \"Error: no test specified\" && exit 1" }, "author": "", "license": "ISC", "dependencies": { "jest-runtime": "20.0.4" } }Create a sample
index.jswith the following contents:Run
node index.js.Expected behavior:
Everything is able to be resolved.
Actual behavior:
We have a missing dependency.
The issue is that
jest-runtimehas a dependency onslashthat it did not declare in itspackage.json, thus PNPM did not link it.jest-runtime.js
{ "name": "jest-runtime", "dependencies": { "babel-core": "^6.0.0", "babel-jest": "^20.0.3", "babel-plugin-istanbul": "^4.0.0", "chalk": "^1.1.3", "convert-source-map": "^1.4.0", "graceful-fs": "^4.1.11", "jest-config": "^20.0.4", "jest-haste-map": "^20.0.4", "jest-regex-util": "^20.0.3", "jest-resolve": "^20.0.4", "jest-util": "^20.0.3", "json-stable-stringify": "^1.0.1", "micromatch": "^2.3.11", "strip-bom": "3.0.0", "yargs": "^7.0.2" }, "devDependencies": { "jest-environment-jsdom": "^20.0.3", "jest-environment-node": "^20.0.3" }, }Many people in the ecosystem are still using npm, and so are likely to continue making mistakes like this in the future. Would it be possible to solve this problem by using some sort of configuration in pnpm which allows a developer to specify certain "bad packages", and either manually add the missing dependency or have an option where pnpm will behave like npm when linking the bad package?
The newer version of jest fixes this issue, but has a different bug that is preventing us from upgrading to it. We only started noticing this issue as we have been exploring migrating our tooling to use pnpm.
The text was updated successfully, but these errors were encountered: