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

rework build system #9710

Merged
merged 20 commits into from
Sep 21, 2021
Merged

rework build system #9710

merged 20 commits into from
Sep 21, 2021

Conversation

paul-marechal
Copy link
Member

@paul-marechal paul-marechal commented Jul 7, 2021

What it does

Fixes: #7275.

Simplify scripts/compile-references.js. TypeScript Server now properly
understand TypeScript references for navigation, so we can simplify our
tsconfig setup.

Rewrite some npm/yarn scripts to do less on install (yarn or yarn install). It will only run the required prepare scripts across our
packages, wire tsconfig.json files and build the TypeScript sources.

To build the example applications you need to run one of yarn browser build, yarn electron build or yarn build:examples.

Linting is done as part of CI, but it won't automatically run locally
anymore. You can run yarn lint to lint your code. For the best
experience you should also enable the ESLint extension in your IDE.

How to test

Installation:

  • yarn alone should not build the example applications anymore, nor should it lint.

Building:

  • yarn build should build TypeScript and both apps.
  • yarn compile should build TypeScript.
  • yarn browser build should build both TypeScript and the browser app.
  • yarn electron build should build both TypeScript and the electron app.
  • yarn build:examples should build both apps.

Watching:

  • yarn watch should watch TypeScript and both example apps.
  • yarn watch:compile should watch only TypeScript.
  • yarn browser watch should watch both TypeScript and the browser app.
  • yarn electron watch should watch both TypeScript and the electron app.

Linting:

  • yarn lint should lint TypeScript sources.

Review checklist

Reminder for reviewers

@paul-marechal
Copy link
Member Author

This is especially useful when using the main repo as a submodule for a different repository such as a Theia extension:

My approach was to clone Theia as a submodule in the extension repo, then add submodules/theia/packages/* and submodules/theia/dev-packages/* to the parent repo yarn workspaces. Finally, I would run the TypeScript compilation in Theia to simply build the TypeScript packages, as this is all that is required for dependents to consume.

yarn link doesn't play nicely with our Theia building tools for some reason, hence why this alternative approach is interesting.

This PR also makes developing a bit more comfortable: only the minimal amount of work required is done, if you need more you need to explicitly trigger it. E.g. building a given app, linting, etc...

@paul-marechal paul-marechal force-pushed the mp/build-clean branch 3 times, most recently from 618b1c2 to 0762ab1 Compare July 7, 2021 21:36
@colin-grant-work
Copy link
Contributor

colin-grant-work commented Jul 9, 2021

This is especially useful when using the main repo as a submodule for a different repository such as a Theia extension

This sounds like it goes some way to addressing some of the pain points described in #9153, as well.

@tsmaeder
Copy link
Contributor

yarn link doesn't play nicely with our Theia building tools for some reason, hence why this alternative approach is interesting.

Could you detail in what way it does not play well with yarn link? it seems to work for me, but I fear I'm missing something.


## Watching

### Watch the TypeScript packages
Copy link
Contributor

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 the term "typescript packages". What would a non-typescript package be, for example?

Copy link
Member Author

Choose a reason for hiding this comment

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

This mostly refers to TypeScript compilation which happens before bundling, so if we have a pure JS extension then there will be nothing to watch.

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand TypeScript package as a package written using TypeScript and requiring transpilation.

Copy link
Contributor

Choose a reason for hiding this comment

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

So it's really "typescript files", because some packages may container both *.js and *.ts files, right? Also, not sure how that works, exactly, but don't we have to reload the code when *.js files change, as well?

Copy link
Member Author

@paul-marechal paul-marechal Jul 13, 2021

Choose a reason for hiding this comment

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

In Theia the watch scripts are related to code transformations. There's basically two steps: TypeScript compilation and Webpack bundling. This documentation refers to the former. JavaScript sources don't require any change during the first step, but bundling still depends on them indeed.

@paul-marechal
Copy link
Member Author

Could you detail in what way it does not play well with yarn link? it seems to work for me, but I fear I'm missing something.

All the times I remember doing yarn link in each Theia package for consumption in third-party Theia extension projects, it ended up with Webpack getting confused and duplicating packages in the output bundles for some reason, leading to runtime errors.

Since we migrated to Webpack 5 maybe this won't be an issue anymore...

So independently from that, this PR still simplifies the compile-references.js script, allows one to manage dependencies without triggering a full build, and cleans up scripts.

@paul-marechal paul-marechal force-pushed the mp/build-clean branch 3 times, most recently from 5abd78f to 4f80f9f Compare July 12, 2021 22:07

### Watch the core and extension packages

To rebuild each time a change is detected run:
To rebuild _everything_ each time a change is detected run:
Copy link
Contributor

Choose a reason for hiding this comment

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

"Everything" being both the regular packages and the example applications?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, both steps: TypeScript compilation and the bundling of both examples.

@msujew
Copy link
Member

msujew commented Aug 18, 2021

@paul-marechal I'll review this, if you're still interested in getting this merged. Would you mind rebasing and fixing merge conflicts?

@paul-marechal
Copy link
Member Author

paul-marechal commented Aug 20, 2021

@msujew thanks for proposing! I just rebased. Note that one of the test lifecycle keeps failing and I don't know why...

edit: I forgot to download the plugins, fixed.

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.

I really like the proposed changes, especially the newer watch options and the removal of yarn lint from every build.

I also have an addition to propose: In my usual workflow when working on Theia I needed to start the server/electron app and watch both typescript and webpack. The latter two are now merged using yarn browser watch and yarn electron watch. I would like to see the former in there as well. Something like this:

"scripts": {
    "start:watch": "concurrently --kill-others -n tsc,bundle,run -c red,yellow,green \"tsc -b -w --preserveWatchOutput\" \"yarn watch:bundle\" \"yarn start\""
}

What's your opinion on that?

@paul-marechal paul-marechal force-pushed the mp/build-clean branch 9 times, most recently from fa34aff to 4d25a63 Compare August 28, 2021 01:46
Sort scripts by name for root, browser and electron packages.
Download plugins as part of `build:examples`.
Cleanups.
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.

@paul-marechal Still fine with merging, everything looks good 👍

@paul-marechal
Copy link
Member Author

I'll just wait on CI to turn green...

@paul-marechal paul-marechal merged commit 23e09c2 into master Sep 21, 2021
@github-actions github-actions bot added this to the 1.18.0 milestone Sep 21, 2021
paul-marechal added a commit that referenced this pull request Sep 24, 2021
Follow up from #9710

When doing refactorings, one might end up renaming/deleting TypeScript
source files. But if a build was run previously, the outputted
JavaScript files will still be in the build output folder. Then
extensions that depend on the refactored package will not fail to build
because the old path still resolves to the lingering JavaScript files.

Add `ts-clean` script that deletes all generated files without a
corresponding source file.

Add the missing compile script to most packages.
@thegecko
Copy link
Member

thegecko commented Oct 1, 2021

Really miss the easy yarn start:browser command in the root to ensure native modules are built correctly and start the browser example.

@msujew
Copy link
Member

msujew commented Oct 1, 2021

@thegecko I think you should be able to run yarn browser start to do exactly the same as the old yarn start:browser.

@thegecko
Copy link
Member

I think you should be able to run yarn browser start

Confirmed, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor experience issues related to the contributor experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

proposal: update scripts to not lint when building
6 participants