-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
Support Yarn backward compatibility mode (node-modules) for Yarn version 2.x or higher #86
Conversation
Thx very much for working on this! |
parcel/package.json
Outdated
"@aurelia/parcel-transformer": /* @if !dev */"latest"/* @endif *//* @if dev */"dev"/* @endif */, | ||
"@parcel/config-default": "^2.6.0", |
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.
I tested yarn pnp loose mode. It didn't help.
I really dislike this patch for parcel's issue.
Instead of doing here which unnecesarily affect users don't use yarn pnp, we can conditionally do "yarn add the-three" in after.js like you did for vscode integration.
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.
Since you commented that this also fixes webpack here, should I move those 2 dependencies in common/package.json
? Or did you mean to execute yarn add
for each package separately, afterwards?
Also, the parcel issue you referenced:
parcel-bundler/parcel#7999
From what I see in the last comment of the issue, there is a bug report:
yarnpkg/berry#4443
And it seems this have been resolve in May this year with:
yarnpkg/berry#4630
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.
Yes I mean yarn add afterwards in after.js. Yarn add will update package.json. But this will only update package.json for users who do use pnp.
You can test the same approach for webpack with pnp, I have not tested that.
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.
To be honest, I am quite confused in the whole parcel and yarn pnp github issues. I am not interested to dig deep . Sorry :-) I had enough experience with yarn and pnpm.
I used yarn and pnpm both for quite some time few years ago, and contributed to them both. But finally I moved back to npm because I really don't want to spend my time to understand the difference anymore. The little saving on install time didn't justify the troubles (both known and unknown) I faced.
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.
@3cp The problem is - default skeletons could be (and could have been) one of the main reasons for poor aurelia
adaption. I'm using aurelia
since 2015 and there were always problems. If 30-50% of combinations for scaffolding do not work, that's not good.
Somewhere during my investigation I read that rule-of-thumb should be - you don't explicitly reference a module in your code which is not explicitly in your package.json
dependencies. Possibly even linter rule... This makes sense to avoid error and applications unexpectedly breaking after update of packages.
In general, if npm
was good enough, there would not be yarn
and pnpm
. If I'm not wrong they are now both shipped with node
installation. Handled via Corepack. So people will eventually move from yarn
v1 (current is 3.2.3) and they will immediately run into all of those issues, same way I did.
Additional note - even after we solve the issue of adding extra packages, Parcel cannot do tree-shaking because of dynamically added module @aurelia/runtime-html
. Maybe this modification should be done earlier in the pipeline, so that tree-shaking works. I mentioned this already, and you can see warning if you add --trace-warnings
flag to parcel
command(s).
Regarding yarn add
in after.js
, it's clear. I will do the change.
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.
I mean we cannot assume user want to do npm install in silent mode. There is usage of private npm registry, user might want to touch .npmrc before deps install.
When design tool, I aid to have less assumption, that enables flexible use cases.
Putting them in question.js also means that's in our offical support. We have to cover yarn and pnpm in CI setup. But we are definitely not keen to officially support future versions of yarn and pnpm.
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.
Wait a min.
With nodeLinker: node-modules
, is not that means we don't need to explicit install @parcel/config-default
?
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.
Yes, I confirmed nodeLinker: node-modules
means it all worked out without explicitly adding of deps.
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.
Yes, I confirmed
nodeLinker: node-modules
means it all worked out without explicitly adding of deps.
@3cp Yes, this option forces yarn
to create node_modules
folder. Backward compatibility mode. Perhaps this would be much simpler approach, instead of support of Plug'n'play. The main goal would be achieved, that project created from skeleton does not break when using yarn
. Maybe this, more complicated approach can be avoided. What do you think?
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.
Why not :)
Our pnpm setup also uses patch in npmrc config yoo.
@@ -79,7 +79,7 @@ test('"after" task only prints summary in unattended mode and here mode', async | |||
); | |||
}); | |||
|
|||
test('"after" task installs deps, and prints summary', async t => { | |||
test('"after" task installs deps with yarn, and prints summary', async t => { |
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.
Please duplicate this test case, one to cover npm, one for yarn.
Unfortunately yarn pnp loose mode didn't help on the parcel issue :-( |
…ge manager issue with Yarn PnP mode.
…e that has previously been choosen to install packages) in `Get Started` instruction.
ee05aee
to
6d719e9
Compare
…peer dependencies found". (Due to "missing peer @parcel/core@^2.8.0")
@3cp After thinking a bit longer about this, I reworked the branch from scratch:
|
Thank you! |
This pull request addresses issue #85.
When development machine has
yarn
version 2.x or higher installed (current version at the moment is 3.x), after developer chooses to installnpm
dependencies usingyarn
,yarn
works in Plug'n'Play mode. This means that nonode_modules
folder in created and allnpm
packages are stored in.yarn
folder unpacked, in originalzip
format.npm start
written in "Getting started" tip does not work.yarn start
is printed now, whenyarn
was picked as package manager.When using Visual Studio Code, for it's TypeScript language service to work Yarn's Editor SDKs extension is needed, to make TypeScript language service (local to the folder) aware of module resolution strategy.For Parcel not to throw errors, two dev-dependencies had to be added to project'spackage.json
: "@aurelia/runtime-html", "@aurelia/router" for direct-routing template and "@parcel/config-default".Now parcel starts/builds project without throwing errors.UPDATE:
It turned out to be a problem to make Yarn's new PnP mode work seamlessly with different IDEs and bundlers. Switching Yarn 2+ to backward compatibility mode with
nodeLinker: node-modules
option, to force it to createnode_modules
folder seems like more stable and simpler option at the moment.