Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chore: fix e2e yarn berry tests #5342

Merged
merged 9 commits into from
Aug 12, 2021
Merged

chore: fix e2e yarn berry tests #5342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
dabd702
disable pnp
slorber Aug 11, 2021
a77f706
test-release support --skip-install + revert to pnp mode
slorber Aug 12, 2021
2590a13
fix yarn canary?
slorber Aug 12, 2021
4d64db6
add YARN_ENABLE_IMMUTABLE_INSTALLS env
slorber Aug 12, 2021
22f4c33
add nodeLinker matrix
slorber Aug 12, 2021
9dc6a59
Update .github/workflows/v2-tests-e2e.yml
slorber Aug 12, 2021
b07199c
polish e2e test workflows
slorber Aug 12, 2021
70bcb09
polish e2e test workflows
slorber Aug 12, 2021
8422f9a
set npm_config_registry
slorber Aug 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 31 additions & 13 deletions .github/workflows/v2-tests-e2e.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,24 +29,32 @@ jobs:
uses: bahmutov/npm-install@v1
with:
install-command: yarn
- name: Setup test-website project against master release
- name: Generate test-website project against master release
run: |
yarn test:build:v2
rm -rf node_modules
KEEP_CONTAINER=true yarn test:build:v2 -s
mv test-website ../test-website
- name: Install test-website project with Yarn v1
run: |
cd ../test-website
yarn install
env:
npm_config_registry: http://localhost:4873
- name: Start test-website project
run: cd test-website && yarn start --no-open
run: cd ../test-website && yarn start --no-open
env:
E2E_TEST: true
- name: Build test-website project
run: cd test-website && yarn build
run: cd ../test-website && yarn build
env:
CI: true
yarn-v2:

yarn-berry:
timeout-minutes: 30
runs-on: ubuntu-latest
strategy:
matrix:
node: ['14']
nodeLinker: ['pnp', 'node-modules']
steps:
- uses: actions/checkout@v2
- name: Use Node.js ${{ matrix.node }}
Expand All @@ -57,24 +65,34 @@ jobs:
uses: bahmutov/npm-install@v1
with:
install-command: yarn
- name: Setup test-website project against master release

- name: Generate test-website project against master release
run: |
KEEP_CONTAINER=true yarn test:build:v2
rm -rf node_modules
- name: Setup test-website project for Yarn v2
KEEP_CONTAINER=true yarn test:build:v2 -s
mv test-website ../test-website
- name: Install test-website project with Yarn Berry and nodeLinker = ${{ matrix.nodeLinker }}
run: |
cd test-website
cd ../test-website

# we have to switch to berry first before setting the version we want
yarn set version berry
slorber marked this conversation as resolved.
Show resolved Hide resolved
# temporary using canary for #5342
yarn set version canary
Comment on lines +79 to +80
Copy link
Contributor

Choose a reason for hiding this comment

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

We've released 3.0.1 with the fix so this can be reverted

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ohhh too late :D

anyway I don't mind staying on canary for now, I could report issues we have with it that do not happen in stable releases


yarn config set nodeLinker ${{ matrix.nodeLinker }}
yarn config set pnpMode loose
Copy link
Contributor

@merceyz merceyz Aug 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
yarn config set pnpMode loose

There are undeclared dependencies that aren't failing the test because of this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I've seen those.
However some are:

  • mdx loader (my fault, will fix)
  • debug using supports-color (not my fault, how to fix?)
  • code copied from CRA using Node.js url. We'll likely delete this code and move back to using CRA code (which also doesn't declare url as dependency btw). Also I'm not totally sure how to fix this. Should I just add npm package url as dependency? How can I know the good version to use considering it's provided by Node.js runtime?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd like to fix those but likely in another PR.
The goal here is just to make our existing CI pass again (which was already using loose mode)

Copy link
Contributor

Choose a reason for hiding this comment

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

debug using supports-color (not my fault, how to fix?)

Nothing to fix, it's an optional peer dependency that isn't provided by the parent so it logs an error but it's fine to ignore and wont be visible in strict mode

code copied from CRA using Node.js url. We'll likely delete this code and move back to using CRA code (which also doesn't declare url as dependency btw). Also I'm not totally sure how to fix this. Should I just add npm package url as dependency? How can I know the good version to use considering it's provided by Node.js runtime?

It's because Webpack 5 doesn't polyfill Node libraries by default, you need to do it yourself

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nothing to fix, it's an optional peer dependency that isn't provided by the parent so it logs an error but it's fine to ignore and wont be visible in strict mode

Thanks

It's because Webpack 5 doesn't polyfill Node libraries by default, you need to do it yourself

I've tried adding alias: {url: 'url'} in Webpack config but the error was still there. It's not clear to me what I should do exactly and the Medium link you gave in some yarn issue didn't make it very clear either

Copy link
Contributor

Choose a reason for hiding this comment

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

You're running into https://github.com/webpack/changelog-v5/tree/b9a346f9090bf2e9f8bc96864a88ed07ad7bc1a4#automatic-nodejs-polyfills-removed so aliasing url to url makes no difference.

Next.js deals with this by aliasing url to native-url https://github.com/vercel/next.js/blob/681d298bdf7048bdbd2790a551d5d231906ede6f/packages/next/build/webpack-config.ts#L125

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see thanks, will try that.
For the mdx-loader issue, unfortunately I can't just add it because it would create a cycle in Lerna build:

lerna WARN ECYCLE @docusaurus/mdx-loader -> @docusaurus/core -> @docusaurus/mdx-loader

I see how I could re-org the code to avoid this cycle but was wondering if there's a way to just tell Yarn that the dependency is here without having to declare it in any deps? peerDependency works but is annoying because all site's parent lockfile would now have to declare @docusaurus/mdx-loader, while we know for sure it is here.

yarn config set npmRegistryServer http://localhost:4873
yarn config set unsafeHttpWhitelist --json '["localhost"]'
yarn config set enableGlobalCache true

yarn install
env:
YARN_ENABLE_IMMUTABLE_INSTALLS: false # Yarn berry should create the lockfile, despite CI env
- name: Start test-website project
run: cd test-website && yarn start --no-open
run: cd ../test-website && yarn start --no-open
env:
E2E_TEST: true
- name: Build test-website project
run: cd test-website && yarn build
run: cd ../test-website && yarn build
env:
CI: true
23 changes: 21 additions & 2 deletions admin/scripts/test-release.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,27 @@ NEW_VERSION="$(node -p "require('./packages/docusaurus/package.json').version").
CONTAINER_NAME="verdaccio"
EXTRA_OPTS=""

if getopts ":n" arg; then
EXTRA_OPTS="--use-npm"
usage() { echo "Usage: $0 [-n] [-s]" 1>&2; exit 1; }

while getopts ":ns" o; do
case "${o}" in
n)
EXTRA_OPTS="--use-npm"
;;
s)
EXTRA_OPTS="--skip-install"
;;
*)
usage
;;
esac
done
shift $((OPTIND-1))


if [ ! -z $EXTRA_OPTS ]
then
echo docusaurus-init extra options = ${EXTRA_OPTS}
fi

# Run Docker container with private npm registry Verdaccio
Expand Down