Skip to content

Fix: Test exception when using global yarn3#14574

Closed
liuxingbaoyu wants to merge 2 commits intobabel:mainfrom
liuxingbaoyu:fix-global-yarn3
Closed

Fix: Test exception when using global yarn3#14574
liuxingbaoyu wants to merge 2 commits intobabel:mainfrom
liuxingbaoyu:fix-global-yarn3

Conversation

@liuxingbaoyu
Copy link
Copy Markdown
Member

@liuxingbaoyu liuxingbaoyu commented May 23, 2022 •
edited by gitpod-io bot
Loading

Q                       A
Fixed Issues? Test exception when using global yarn3
Patch: Bug Fix? √
Major: Breaking Change? ×
Minor: New Feature? ×
Tests Added + Pass? ×
Documentation PR Link
Any Dependency Changes?
License MIT

@babel-bot
Copy link
Copy Markdown
Collaborator

babel-bot commented May 23, 2022 •
edited
Loading

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/52447/

nicolo-ribaudo
nicolo-ribaudo reviewed May 24, 2022
View reviewed changes
scripts/integration-tests/utils/local-registry.sh Outdated
if [[ $(YARN_IGNORE_PATH=1 yarn --version) == "1"* ]]; then
YARN_IGNORE_PATH=1 yarn global add verdaccio-memory@~10.0.0
else
yarn dlx verdaccio-memory@~10.0.0
Copy link
Copy Markdown
Member

@nicolo-ribaudo nicolo-ribaudo May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this just like npx? i.e. it doesn't make the package globally available; it just executes it.

Copy link
Copy Markdown
Member Author

@liuxingbaoyu liuxingbaoyu May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the global install was removed in yarn3.

Copy link
Copy Markdown
Contributor

@merceyz merceyz May 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicolo-ribaudo Correct, in this case it will download verdaccio-memory and look for a binary with the same name, it wont be able to locate one and throw.

@JLHwung JLHwung requested a review from nicolo-ribaudo July 6, 2022 19:02
@nicolo-ribaudo
Copy link
Copy Markdown
Member

nicolo-ribaudo commented Jul 7, 2022

This PR does not do what you think it does.

What we need is that Yarn should install verdaccio-memory and make it globally available, so that verdaccio can then use it. What this PR does is that it tries to execute verdaccio-memory (and as mentioned by @merceyz it fails), without installing it.

I don't remember why we need verdaccio-memory, maybe @juanpicado knows? (it originally came from juanpicado#1)

@merceyz
Copy link
Copy Markdown
Contributor

merceyz commented Jul 7, 2022 •
edited
Loading

@nicolo-ribaudo Instead of using nohup npx have you (the Babel team) considered using the docker container instead?

@nicolo-ribaudo
Copy link
Copy Markdown
Member

nicolo-ribaudo commented Jul 7, 2022

The main reason is that I have almost zero docker experience and I thus I didn't think about it 😛
If someone wants to update our e2e tests to use docker (either some from the team, or external) I'd be really happy about that because it also makes it easier to run them locally.

@liuxingbaoyu
Copy link
Copy Markdown
Member Author

liuxingbaoyu commented Jul 7, 2022 •
edited
Loading

I will try to research it. (Probably not docker though, since it's not particularly convenient on Windows, and also feels like it might be a bit slow in CI)

@juanpicado
Copy link
Copy Markdown
Contributor

juanpicado commented Jul 7, 2022 •
edited
Loading

This PR does not do what you think it does.

What we need is that Yarn should install verdaccio-memory and make it globally available, so that verdaccio can then use it. What this PR does is that it tries to execute verdaccio-memory (and as mentioned by @merceyz it fails), without installing it.

I don't remember why we need verdaccio-memory, maybe @juanpicado knows? (it originally came from juanpicado#1)

I think you are not using it https://github.com/babel/babel/blob/main/scripts/integration-tests/verdaccio-config.yml#L9 so I guess it's safe to be removed. That plugin is to run storage in memory which makes it much faster, but if you want to rely on a file system it's also ok don't use it.

If you were using it, the storage: ./some-path won't be used.

@liuxingbaoyu
Copy link
Copy Markdown
Member Author

liuxingbaoyu commented Jul 7, 2022 •
edited
Loading

I'm not sure if it's working, but it should be able to be removed without much impact.

https://github.com/liuxingbaoyu/babel/runs/7231597398?check_suite_focus=true

In e2e-publish, the time increased by half a minute - 1 minute, and the overall CI speed did not change much.

renew:

https://github.com/liuxingbaoyu/babel/runs/7231916640?check_suite_focus=true

It can be seen from this that there will be no impact.

@liuxingbaoyu liuxingbaoyu mentioned this pull request Jul 8, 2022
Merged
@liuxingbaoyu
Copy link
Copy Markdown
Member Author

liuxingbaoyu commented Jul 8, 2022

Closing this as it was erroneously included in https://github.com/babel/babel/pull/14701.😰

@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 8, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JLHwung JLHwung JLHwung approved these changes

@nicolo-ribaudo nicolo-ribaudo Awaiting requested review from nicolo-ribaudo

+1 more reviewer

@merceyz merceyz merceyz left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

outdated A closed issue/PR that is archived due to age. Recommended to make a new issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@liuxingbaoyu @babel-bot @nicolo-ribaudo @merceyz @juanpicado @JLHwung