-
Notifications
You must be signed in to change notification settings - Fork 669
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
Fix VSCode Launch Task and Monorepo builds #193
Conversation
- Removed commands from sub-package: same can be achieved from the root - Added build and no-build variants. In case separate watcher is already running, no need to build. - Changed watch to build (no hot-reload in vscode by default) - Show the build in console Didn't update yarn.lock, seems to want to add foam-core@0.3.0-alpha.0, but not really related to this change Note: the extension tests are failing due to `Cannot find module '...foam-vscode/out/test/suite/index'`. However that shouldn't be related to this change either.
https://www.typescriptlang.org/docs/handbook/project-references.html#build-mode-for-typescript This configures tsc to use multiple project roots (vscode, core) and run incremental builds on changes to each projects. This will necessitate us running the core in commonjs build mode, which is fine for testing and deployment to node environments, but won't work in the browser, so foam-core will need a separate build config for UMD builds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clearly an improvement to the previous setup, good job!
Would it still make sense to remove the tasks.json and launch.json from foam-vscode? Now these contain duplicate definitions to files in the root, and looks like they already started to drift apart (?).
In the first phase I'd keep everything running solidly from root only, and if these packages really become packages in their own right (e.g. with introduction of foam-vim or something like that 😄 ), then could start replicating them into different places.
{ | ||
"type": "node", | ||
"request": "launch", | ||
"name": "Workspace Manager tests", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping some kind of entry point to debug the core tests would be nice? We could put this also into foam-core launch.json, but it would probably be somewhat annoying to need to load that folder into separate vscode instance. Thus I'd still keep this here (and name it e.g. to "foam-core unit tests" or something similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I didn't think of debugging tests, I was making the assumption that we would just run tests from the command line but that is clearly insufficient!
@jojanaho I've removed the duplicated .vscode directory entirely from packages/foam-vscode as not to encourage its use in workspace root. I've also re-added the launch task for core tests, but they're not running. When are they expected to be ran? At the same time as the extension host? If so, I'm not seeing the tests execute, possibly because the watch mode terminal is foregrounded, and the test task actually runs in the background? Feel free to try to fix that or merge now, and we can address it later. The default test task can still be ran, and works. |
I guess the main task for the foam-core launch target is to be able to debug the core unit tests; now I can set a breakpoint and land there, so that's great! What's not so great is that on debugger exit, we get:
With a quick search, I was able to find this - so it might be related to a bug in Node. However that's not too critical, since we still can inspect the actual results of the test by running
|
Builds upon and supercedes @jojanaho's work in #187. If this approach works, the other PR can be closed as this contains the commits from it.
The big change here is that foam-vscode build also now buils the foam-core package, so that we no longer need to run a separate watcher process for foam-core while developing.
This relies on the TSC "build mode", and "references" field, which we already had in place. (eb22cd3)
https://www.typescriptlang.org/docs/handbook/project-references.html#build-mode-for-typescript
This will necessitate us running the core in commonjs build mode that generates a commonjs entry point file instead of a
.cjs.development.js
bundle as before, which is fine for testing and deployment to node environments (such as vs code), but won't work in the browser, so foam-core needs a separate build config for UMD builds (d642738)Test plan
First use per VSCode session
Subsequent uses per VSCode session