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

Use npm instead of yarn to build Theia #14481

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

tsmaeder
Copy link
Contributor

@tsmaeder tsmaeder commented Nov 19, 2024

What it does

This PR rewrites the Theia build process to use npm in stead of yarn. This required a couple of changes:

Contrary to yarn, npm does not invoke the prepare scripts of workspace packages in the order of their dependencies (npm/cli#3034). So instead of relying on yarn install to invoke the necessary build scripts, we have to invoke the build process explicitly after the npm install. The build process is built on this logic:

  1. Each package has a script called "build". The goal of that target is to fully build all artifacts this repo produces. For regular Theia packages, this is just incrementally compiling the typescript sources of the package an it's dependencies, but for applications, for example, it means creating the application bundle. So calling "build" in an application package should be all that's needed in regular development
  2. To build the repo initially after check-out, there are a couple of additinal steps necesary:
    • compile build tools (re-exports package)
    • generate re-exports where applicable
    • compile all packages
    • build alll application packages
      This can be achieved by invoking npm run build in the repo root.

Fixes #13948

Contributed on behalf of STMicroelectronics

How to test

Make sure we can build Theia as described above and the apps work fine.

Follow-ups

Review checklist

Reminder for reviewers

@dankeboy36
Copy link
Contributor

Fixes #13948

Possible related: #775

@tsmaeder
Copy link
Contributor Author

@msujew @sdirix the PR checks now pass and it's ready to be reviewed minus doc.

Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

So far it works very well.

I used npm ci to install. I tested all applications and was able to build and start all of them. I also tested watching for the browser application. Tests also completed successfully.

Linting is currently broken but this is already mentioned in the current PR description.

@theia/request: > @theia/[email protected] lint
@theia/request: > theiaext lint
@theia/request: $ eslint --cache=true --no-error-on-unmatched-pattern=true "{src,test}/**/*.{ts,tsx}"
@theia/request: /bin/sh: 1: node: not found
@theia/request: Oops! Something went wrong! :(
@theia/request: ESLint: 8.57.1
@theia/request: SyntaxError: Failed to load plugin '@theia' declared in '.eslintrc.js » ../../configs/build.eslintrc.json » ./base.eslintrc.json': Unexpected end of JSON input
@theia/request:     at JSON.parse (<anonymous>)
@theia/request:     at PackageReExports.FromPackageSync (/home/user/git/theia/dev-packages/private-re-exports/lib/package-re-exports.js:127:47)
@theia/request:     at Object.<anonymous> (/home/user/git/theia/dev-packages/private-eslint-plugin/rules/shared-dependencies.js:24:40)

@@ -1,7 +1,6 @@
root = true

[*]
insert_final_newline = true
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep this? Why remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we now have the preference "files.insertFinalNewline" which acomplishes the same thing.

.github/workflows/ci-cd.yml Outdated Show resolved Hide resolved
dev-packages/private-ext-scripts/package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
dev-packages/private-ext-scripts/package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Works pretty well, but there are still a few calls to yarn littered throughout the build scripts:

There might be more. See also below.

packages/electron/package.json Show resolved Hide resolved
scripts/compile-references.js Outdated Show resolved Hide resolved
@tsmaeder tsmaeder marked this pull request as ready for review December 18, 2024 15:24
@tsmaeder tsmaeder requested a review from sdirix December 18, 2024 15:24
@tsmaeder
Copy link
Contributor Author

@JonasHelming in response to your question elsewhere, the documentation on https://theia-ide.org/ mostly uses the yeoman generator output and the theia-ide repository as examples. While the output of the generator and theia-ide still use yarn, it makes no sense to update the documentation.

@tsmaeder tsmaeder requested a review from msujew January 3, 2025 10:20
Fixes eclipse-theia#13948

Contributed on behalf of STMicroelectronics

Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
@tsmaeder
Copy link
Contributor Author

tsmaeder commented Jan 6, 2025

@msujew @sdirix could I get some love for this one, please? If we merge it, we should merge it early in the release cycle.

Copy link
Member

@msujew msujew 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 know. Everything seems to work as expected without relying on yarn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs merge
Development

Successfully merging this pull request may close these issues.

Migrate away from yarn v1
4 participants