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.

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

[lerna] upgrade to recent version, refactoring of package.json "scripts" entries #1016

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

marcdumais-work
Copy link
Contributor

@marcdumais-work marcdumais-work commented Sep 15, 2023

Description

Updated lerna from v4.x to 7.x, getting rid of several big security vulnerabilities, that were present in the older version. Also updated their repo's root README to do small adjustments, related to the changes. I used the README to test too, replicating the contained commands.

More details:

root package.json and electron/browser package.json:

Cleaned-up the "scripts" section.

Some uses of lerna in there were not working any more, so they were replaced with a non-lerna alternative. Since I was already in there, I took the opportunity to refactor the "scripts" section. I attempted to keep the "public API" the same - i.e. so the the commands that are documented in the README and used elsewhere in the repo should still work the same.

For instance:

  • Added convenience shortcuts to call scripts in browser and electron sub-packages, called respectively "electron" and "browser", rather than having to use the long form every time (yarn --cwd )
  • Moved to the specific application, some scripts that ought to be defined there, leaving the root entries as shortcuts, to call the specific implemetation(s).
  • Introduced the "concurrently" dev-dependency - it helps start two or more processes from one "script" entry, format their output so it's readable and easy to distinguish what came out of which process. It also can watch all started processes and make sure that if one exits, the others are killed too (using the "--kill-others" option). This is used in the "scripts" entries that start both one of the viewer application(browser or electron) and the trace server.
  • To avoid potential issues, modified the "workspaces" section to specifically name the browser and electron applications separately rather than use a glob (*): the reason is that the "plugins" folder is created in the same "examples" folder, and could be considered a yarn workspace entry on its own, which we do not want.

lerna.json:

updated, according to the new version used.

electron-builder config:

Corrected relative path to the plugins folder, so that it will be included in packages, such as AppImage and deb on Linux. Also updated currently used Electron version (23.6.0).

How to test?

  1. some of the commands below will modify your ~/.npmrc file. If you wish, you can do a backup copy and restore it at the end.
  • (optional) $> cp ~/.npmrc ~/.npmrc-orig
  1. We need a local registry to test publishing without polluting the public registry.
  • $> npm add -g verdaccio
  • $> verdaccio # start verdaccio in a terminal, then continue in another one (main)
  • in main terminal, set local registry as temporary default
    - $> npm config set registry http://localhost:4873/
    - $> yarn config set registry http://localhost:4873/
    - $> npm adduser --registry http://localhost:4873/ # use bogus user/credentials. e.g.: test/test/[email protected]
  1. simulate "latest" release from the PR commit
  • $> git clean -ffdx && yarn && yarn && yarn test
  • $> npx lerna publish --registry=http://localhost:4873 --exact --no-git-tag-version --no-push
    • select "minor" when prompted
  • verify that publishing worked as expected, e.g. without errors. Note the name/versions of the published packages. e.g.:
  1. Test a local Theia application, using the published packages
  • temporarily modify the browser and electron Theia apps' package.json, to use the just published version of "theia-traceviewer". e.g. "theia-traceviewer": "0.2.0" (instead of 0.1.0)
  • in root package.json, temporarily remove "theia-extensions/*" from the yarn workspace (else local version 0.1.0 will be picked-up
  • rebuild the Theia apps: $> git clean -ffdx && yarn && yarn build:examples && yarn download:plugins && yarn download:server && yarn download:sample-traces
  • start the example app and verify it's working fine
    • $> yarn start:electron and/or $> yarn start:browser
    • In the application, verify that the expected version of "theia-traceviewer" extension is used (top menu: Help -> About)
    • Open a trace, ...
  1. simulate next release - cleanup any checked-out file left before proceeding
  • $> git reset --hard && git clean -ffdx && yarn && yarn test
  • $> npx lerna publish --registry=http://localhost:4873 --exact --canary minor --preid=next.$(date -u '+%Y%m%d%H%M%S').$(git rev-parse --short HEAD) --dist-tag=next --no-git-tag-version --no-push --yes
  • verify that publishing worked as expected, e.g. without errors. Note the name/versions of the published packages. e.g.:
  1. Test a local Theia application, using the published packages
  • temporarily modify the browser and electron Theia apps' package.json, to use the just published version of "theia-traceviewer". e.g. "theia-traceviewer": "0.2.0-next.20230918212601.f4c70ca.375" (instead of 0.1.0)
    • note: in the version, anything after the plus sign ("+") can be ignored
  • in root package.json, temporarily remove "theia-extensions/*" from the yarn workspace (else local version 0.1.0 will be picked-up
  • rebuild the Theia apps: $> git clean -ffdx && yarn && yarn build:examples && yarn download:plugins && yarn download:server && yarn download:sample-traces
  • start the example app and verify it's working fine
    • $> yarn start:electron and/or $> yarn start:browser
    • In the application, verify that the expected version of "theia-traceviewer" extension is used (top menu: Help -> About)
    • Open a trace, ...
  1. un-setup
  • when all done, unset local registry (else entries related to it will be included in yarn.lock as you continue working on something else:
  • $> npm config delete registry http://localhost:4873/
  • $> yarn config set registry https://registry.npmjs.org/
  • (optional) restore original .npmrc: $> rm ~/.bashrc && mv ~/.bashrc.orig ~/.npmrc
  • kill verdaccio in the other terminal (Ctrl-c)

yarn audit

Before this PR (master):
image

After:
image

@marcdumais-work marcdumais-work force-pushed the upgrade-lerna branch 4 times, most recently from bbe7983 to f4c70ca Compare September 18, 2023 21:38
@marcdumais-work marcdumais-work marked this pull request as ready for review September 18, 2023 21:38
Copy link
Collaborator

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

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

Thanks for the lerna upgrade. I was able to test and publish locally.

Please also update the gitpod.yml accordingly.

README.md Outdated
3. Now you are ready to build the application:

```bash
yarn && yarn all # install npm dependencies and build all
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure about this. I'm used to just do yarn and then either execute yarn start:browser or yarn start:electron. All other users did the same. Having to do yarn all won't be known and we might get questions about failing yarn start:browser. Is there a way to keep yarn that builds all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can continue to use "prepare" like that if we chose. But the trend is going in the other direction. From what I understand, yarn2 took measures to prevent "prepare" being used like this - if/when we eventually upgrade, I think it will be necessary to at least do yarn && yarn prepare. See: yarnpkg/berry#2305 (comment)

For now I will revert that change in the form of an extra commit on top, that we can squash or remove, before merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can also update the PR description and commit message, if we want to preserve current "yarn prepare" capabilities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bhufmann can you have another look? Consider the two commits together, as well as my comments above, and let me know if that's what you want. If so, I will squash the commits together and update the commit message

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think for now we should preserve the current "yarn prepare" capabilities. You can open a separate issue tracker and PR for changing the behaviour. I think it would be more transparent for the developers to follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have pushed a new version, that preserves backward compatibility of the currently used (AFAIK) root package.json "scripts" - let me know what you think.

@marcdumais-work
Copy link
Contributor Author

Thanks for the lerna upgrade. I was able to test and publish locally.

Please also update the gitpod.yml accordingly.

Done in the first commit. The only change I think was necessary was to adapt to use yarn && yarn all. The change is reverted in the second commit, where it's again possible to build using a plain yarn command.

@marcdumais-work marcdumais-work changed the title [lerna] upgrade to recent version [lerna] upgrade to recent version, refactoring of package.json "scripts" entries Sep 20, 2023
@marcdumais-work marcdumais-work force-pushed the upgrade-lerna branch 3 times, most recently from dfdfde7 to e099bd2 Compare September 20, 2023 19:33
…ts" entries

Updated lerna from v4.x to 7.x, getting rid of several big security vulnerabilities,
that were present in the older version. Also updated ther repo's root README to do
small adjustments, related to the changes. I used the README to test too, replicating
the contained commands.

More details:

root package.json and electron/browser package.json:

Cleaned-up the "scripts" section.

Some uses of lerna in there were not working any more, so they were replaced with a
non-lerna alternative. Since I was already in there, I took the opportunity to
refactor the "scripts" section. I attempted to keep the "public API" the same - i.e.
so the the commands that are documented in the README and used elsewhere in the repo
should still work the same.

For instance:

- Added convenience shortcuts to call scripts in browser and
electron sub-packages, called respectively "electron" and "browser",
rather than having to use the long form every time (yarn --cwd <folder> <command>)
- Moved to the specific application, some scripts that ought to be defined there,
leaving the root entries as shortcuts, to call the specific implemetation(s).
- Introduced the "concurrently" dev-dependency - it helps start two or more processes
from one "script" entry, format their output so it's readable and easy to distinguish
what came out of which process. It also can watch all started processes and make sure
that if one exits, the others are killed too (using the "--kill-others" option). This
is used in the "scripts" entries that start both one of the viewer application (browser
or electron) and the trace server.
- To avoid potential issues, modified the "workspaces" section to specifically name the
browser and electron applications separately rather than use a glob (*): the reason is
that the "plugins" folder is created in the same "examples" folder, and could be
considered a yarn workspace entry on its own, which we do not want.

lerna.json:

updated, according to the new version used.

electron-builder config:

Corrected relative path to the plugins folder, so that it will be included in packages,
such as AppImage and deb on Linux. Also updated currently used Electron version
(23.6.0).

Fixes #1013

Signed-off-by: Marc Dumais <[email protected]>
Copy link
Collaborator

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the contribution!

@marcdumais-work marcdumais-work merged commit cf82da7 into master Sep 21, 2023
3 checks passed
@marcdumais-work marcdumais-work deleted the upgrade-lerna branch September 21, 2023 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants