Skip to content
This repository was archived by the owner on Apr 13, 2025. It is now read-only.

Fix/npm workspaces #268

Merged
merged 7 commits into from
Oct 23, 2021
Merged

Fix/npm workspaces #268

merged 7 commits into from
Oct 23, 2021

Conversation

TimTechDev
Copy link
Contributor

Should fix #263

Contents

  • Remove lerna
  • Add npm-workspaces
  • New build process using typescript project references
  • Remove webpack ts-loder
  • Add update-paths.mjs
  • Fix some TS errors
  • Adapt pipeline

Time savings

On: Ubuntu 21.04 x86_64
Using: AMD Ryzen 7 4700U (8) @ 2.000GHz

Install Bootstrap Rebuild
Before 7.185s 14.386s 1m0.409s
After 18,262s - - - 24,252s

Before

$ time npm ci
real    0m7.185s
user    0m6.718s
sys     0m2.290s

$ time npm run bs
real    0m14.386s
user    0m18.274s
sys     0m3.085s

$ time npm run rebuild
real    1m0.409s
user    6m39.122s
sys     0m26.532s

After

$ time npm ci
real    0m18,262s
user    0m24,543s
sys     0m5,145s

$ time npm run rebuild
real    0m24,252s
user    0m38,086s
sys     0m2,221s

To make transition to npm-workspaces
and especially ts project references easier.

Signed-off-by: Tim_Tech_Dev <Tim_Tech_Dev@protonmail.com>
- Remove lerna
- Add npm-workspaces
- New buildprocess using typescript project references
- Remove webpack ts-loder
- Add update-paths.mjs
- Fix some ts errors

should fix #263

Signed-off-by: Tim_Tech_Dev <Tim_Tech_Dev@protonmail.com>
@hlxid hlxid self-requested a review October 18, 2021 18:29
Copy link
Member

@hlxid hlxid left a comment

Choose a reason for hiding this comment

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

Just did a brief overview of this PR. Thank you very much for your hard work in advance!

Here's what I found so far:

  • we need to update nodecg-io-cli to add the service directory to the nodecg configuration when creating a development install. Running the bootstrap script is also not needed anymore and needs to be removed. (can also be after the PR is merged but it would be nice to be able to push an update once we merge this)
  • providing the path object inside compilerOptions is to my understanding not necessary. Because we are inside a workspace the typescript compiler can use the normal node module resolution algorithm to figure out imports
  • monaco-editor in the core and debug dashboard cannot be loaded because they are not inside the node_modules directory of the corresponding dashboard anymore. Instead monaco-editor is now also put in the node_modules directory inside the repository root. To fix this I would propose to mount the root node_modules using a nodecg mount and then importing it from there
  • I personally would not check in the samples/tsconfig.json, services/tsconfig.json and utils/tsconfig.json because they are generated at build time. Then we also wouldn't need to run prettier on each build which saves some time. I don't see any real advantage that we would have, if we would commit them
  • .scripts/create-service.py needs to be update to create services in the new services directory and to update the package name in the tsconfig.json of the generated sample. The "create a service"-guide also needs updating but may be replaced with a guide explaining how the service creation script is used

@TimTechDev
Copy link
Contributor Author

TimTechDev commented Oct 20, 2021

Thanks for your response. I will create two pull request accompanying this one (docs and CLI), but they are not ready yet, and I'm quite busy atm. So the stuff on the To-do list:

  • Update CLI (remove bootstrap & add nodecg-io/services to config)
  • Update create-service.py and add documentation for it
  • Update the Docs overall
  • Fix dashboard and cores dependency problem

@TimTechDev
Copy link
Contributor Author

monaco-editor in the core and debug dashboard cannot be loaded because they are not inside the node_modules directory of the corresponding dashboard anymore.

Well, I can build and use the dashboard in the current branch, so I can't find this problem. May you explain the issue more clearly? Thank you.

- Remove type mapping
- Update create_service.py
- Update update-paths.mjs
- Remove build scripts from nodecg-io-serial

Signed-off-by: Tim_Tech_Dev <Tim_Tech_Dev@protonmail.com>
@TimTechDev
Copy link
Contributor Author

Why does the lint fail?

@hlxid hlxid self-requested a review October 21, 2021 04:32
@hlxid
Copy link
Member

hlxid commented Oct 21, 2021

monaco-editor in the core and debug dashboard cannot be loaded because they are not inside the node_modules directory of the corresponding dashboard anymore.

Well, I can build and use the dashboard in the current branch, so I can't find this problem. May you explain the issue more clearly? Thank you.

Hmmm, for me it fails because http://localhost:9090/bundles/nodecg-io-debug/monaco/monaco-editor/min/vs/loader.js returns 404. Maybe it is still in the node_modules directory for you. Can you try to remove all node_modules directories and re-running npm install and check if then pops up? Otherwise I can fix it and do a PR aswell.

Why does the lint fail?

Previously running npm install or npm ci would only install the dependencies that are defined in the root package.json and all other dependencies would be installed when npm run bootstrap was run.
With npm workspaces npm install already installs all dependencies, including those in all services. Some services like midi use native modules which require some os packages which are not installed in the lint workflow. To fix this you can copy the apt install ... step from the CI to the lint workflow.

@TimTechDev
Copy link
Contributor Author

I could reproduce the error now.

- Use `nodecg.mount`
- Remove tsconfig files wich are generated at build time
- Fix lint CI
- Format CI

Signed-off-by: Tim_Tech_Dev <Tim_Tech_Dev@protonmail.com>
Left some code from testing :|
Copy link
Member

@hlxid hlxid left a comment

Choose a reason for hiding this comment

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

Can you please update the directories inside the repository field of all service packages like you documented in https://github.com/codeoverflow-org/nodecg-io-docs/pull/75/files#diff-dbc6349c40b8736d2697bba02f2acc769d89b3c6636d0b268ad6b1028cb29eaeL34?
After that this PR should be finally ready to be merged 🎉

- Thanks daniel0611

Signed-off-by: Tim_Tech_Dev <Tim_Tech_Dev@protonmail.com>
Signed-off-by: Tim_Tech_Dev <Tim_Tech_Dev@protonmail.com>
@TimTechDev TimTechDev requested a review from hlxid October 21, 2021 19:48
Copy link
Member

@hlxid hlxid left a comment

Choose a reason for hiding this comment

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

Thank you very much for you PR. I've found two more things, but will merge this anyways. You can create a follow-up PR for those.
Sorry I've taken so long to review this.

"watch": "lerna run --parallel watch",
"prewatch": "npm run update",
"watch": "tsc -b -w",
"postwatch": "npm run watch --workspaces --if-present",
Copy link
Member

Choose a reason for hiding this comment

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

postwatch sadly won't work as intented because watch never finishes. Therefore postwatch will never be executed. Also running watch in each directory also has its problems because they are executed one after another and don't terminate, so only the first one would be executed.
To be honest I think it is fine if only the code added to the root tsconfig.json is watched, but having it watch everything would obiously be nice.

"build": "lerna run --stream build",
"clean": "tsc -b --clean",
"update": "node .scripts/update-paths.mjs",
"postupdate": "prettier -w --loglevel silent \"./**/*.json\"",
Copy link
Member

Choose a reason for hiding this comment

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

I assume you've added this to ensure that the generated tsconfig.jsons have proper formatting. Can this be removed as we're not commiting the generated tsconfig.jsons anymore? Would be nice to be able to shave off another ~2 seconds off from the build time.

@hlxid hlxid merged commit 305bba6 into codeoverflow-org:master Oct 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ditch lerna & add auto-updating for npm with dependabot
2 participants