-
Notifications
You must be signed in to change notification settings - Fork 1
Add pnpm resolve plugin #22
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
base: main
Are you sure you want to change the base?
Conversation
|
Hello @devchakraborty, I think that a better approach than reworking the apos vite plugin would be to use the pnpm public-hoist-pattern feature that allow to hoist only the packages you need. Here is an example of a .npmrc file to put at the root of your project, (can be added in the package.json I think too): This will hoist eslint packages (if needed by IDEs), all packages with a name starting with @apostrophecms, and also vue-advanced-cropper because we import files directly (css files) not allowed by pnpm strict mode. With this config you'll be able to run you apostrophe project, I just tested it :) Another advice for pnpm, you can configure allowed lib that perform postinstall scripts, for apostrophe you might need sharp (config to put in the package.json): "pnpm": {
"onlyBuiltDependencies": [
"sharp"
]
} |
|
Hi @ValJed - adding apostrophe packages to the public hoist pattern for pnpm does not fix the build for us because it is the transitive dependencies that we are failing to resolve, not the apostrophe packages themselves. My build error on pnpm v10 is: Hoisting |
|
@devchakraborty Ok I see, I can reproduce the problem with a pnpm monorepo indeed. |
|
@devchakraborty I'd like to test the change first before discuss it with you (and look closely at the implementation you offer). |
|
Hi @myovchev, any updates? We have applied this change as a pnpm patch internally so we are no longer blocked, but we would prefer not to have this patch around indefinitely. Hoisting is not a sustainable option in our monorepo either. |
|
Hey @devchakraborty sorry for the lack of udpates. @myovchev isn't available these days, he is responsible for this module so it's better if he takes care of it. Should be pretty soon, cool it's not a blocker for you!! |
Hey @devchakraborty, sorry for the delay. I'm back and I'll be working on this one now. |
myovchev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devchakraborty This is a good base but can't be a complete solution due to many missing (and failing) parts. I decided to create a separate branch that builds upon your idea, but also adds some internals and handles edge cases (e.g. configurations, defaults, changelog, docs) - #23
I also created a playgrounds based on our vite-demo repo. In the PR details, among detailed explanations about what we should add as a solution and maintain in the future, are also details about how to run the change against pnpm simple demo project and pnpm workspace project (aka monorepo).
Please look closely at the PR, do tests and experiment in your own setup and environment to see if it behaves as expected. We can collborate further how we proceed with the final contribution after that.
Summary
In
pnpm, unlike other package managers, by default dependencies are not hoisted to the highestnode_modulesfolder possible. As a result, onpnpmthe admin UI is unable to build; the copied source in theapos-buildfolder is not able to resolve its transitive dependencies.This PR adds a plugin that helps module resolution in a
pnpmcontext. Given a list of direct dependency packages, it searches those packages' dependencies as needed. This should be a no-op outsidepnpmbecause vite and rollup get a chance to resolve the module first; hoisted dependencies will always be found before this plugin searches elsewhere.What are the specific steps to test this change?
pnpm.What kind of change does this PR introduce?
Make sure the PR fulfills these requirements:
Other information: