-
Notifications
You must be signed in to change notification settings - Fork 6.2k
Not planned
Labels
enhancementSome improvement that isn't a featureSome improvement that isn't a feature
Milestone
Description
What is your suggestion?
Use npm
for release:standalone
in CI.
Why do you want this feature?
It will respect our shrinkwrap file. See logs here:
warning npm-shrinkwrap.json found. This will not be updated or respected. See https://yarnpkg.com/en/docs/migrating-from-npm for more information.
Are there any workarounds to get this functionality today?
No
Are you interested in submitting a PR for this?
I can if no one else wants to.
Metadata
Metadata
Assignees
Labels
enhancementSome improvement that isn't a featureSome improvement that isn't a feature
Type
Projects
Relationships
Development
Select code repository
Activity
jsjoeio commentedon Sep 6, 2022
@edvincent tagging you because I think you'd find this interesting. Also maybe you want to help with this? If not, no worries :) Seems like a one-liner.
edvincent commentedon Sep 6, 2022
That's the one where I was suggesting to use
npm ci
right? Happy to have a look yup, was already in my to-do from #5071 (comment)As a side-comment, that error
is just cosmetic. In
release:standalone
in the CI, we have both ayarn.lock
and annpm-shrinkwrap.json
, so bothyarn
andnpm
could be used.But yes, way better to start using
npm ci
to keep it consistent with what other users would use.Can I sign-up to do this during the next week or so, or is this more urgent?
edvincent commentedon Sep 6, 2022
Oh nvm, you mean actually use
npm run
to run the tests etc...? Because I think you already merged the one to install it withnpm
?jsjoeio commentedon Sep 6, 2022
that'd be awesome! absolutely no rush so whenever you have time :)
Good question! Our thinking was to replace it here:
edvincent commentedon Sep 6, 2022
So, I don't think we should do that? I think the line between
yarn
andnpm
should be development of code-server vs install of code-server itself.Or in other words, any command starting from the root of the project (i.e. development) should be using
yarn
, and if during the development we need to installcode-server
(i.e. go withinrelease
orrelease-standalone
), that's where we should usenpm
.Which is basically the current state?
Specifically, because we don't have an
npm-shrinkwrap.json
tool at the root of the project.That being said, since your #5533, we now should be clear to remove the
yarn.lock
(or more specifically, notrsync
them into the nested directories) as we'll always use npm there?jsjoeio commentedon Sep 6, 2022
I do agree we should have the distinction. It would be nice if we used one instead of mixed them though. I wonder if instead we should move development to
npm
then 🤔 What do you think?cc @code-asher
That makes sense to me!
edvincent commentedon Sep 6, 2022
I don't think you can ;( Because
vscode
only comes with ayarn.lock
file/expectsyarn
tasks... Actually, that being said, I believe the latest versions ofnpm
respects theyarn.lock
file.I think that was the reason
code-server
was moved to useyarn
back in the day no?jsjoeio commentedon Sep 6, 2022
Ahhh I forgot about that 🤦🏼♂️
That's before my time but I bet you're right.
code-asher commentedon Sep 7, 2022
VS Code embeds
yarn
in a bunch of places so that might throw a wrench in some things, for example their postinstall and build scripts. There is also a check in their preinstall script that forbids the use of npm. This could all be patched of course but I am not sure we should.Agreed on removing the
yarn.lock
rsync!The
npm-shrinkwrap.json found
warning was confusing because it made it seem like we were usingyarn
to install the dependencies in the standalone release even though we runnpm install
so the thinking was that maybeyarn run
provides annpm
shim that actually runsyarn
or something tricky like that.I tested a bit and
npm_config_user_agent
showsyarn/1.22.19 npm/? node/v16.16.0 linux x64
(in the postinstall script) even though we install withnpm install
so we end up using yarn.So maybe
npm
avoids overwriting that or maybeyarn
does do some kind of trickery here. Usingnpm run
is one fix but maybe there is a better one.3 remaining items