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.

Sign up for GitHub

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

Jump to bottom

Fix `./run gui watch` #6212

Merged

mergify merged 11 commits into develop from wip/sb/fix-gui-watch

Apr 11, 2023

Collaborator

somebody1234 commented Apr 5, 2023 •

edited

Pull Request Description

Fixes Watch does not work anymore #6168
Removes enso-copy-plugin in favor of an inline plugin
- It was only used in one place anyway
- It is probably necessary since I've "fixed" it by adding all files as entrypoints (I'm not quite sure why it wasn't working with the fix with enso-copy-plugin...)
Adds live reload (back) to content/

Important Notes

To QA:
Mandatory:

./run gui watch --skip-version-check --skip-wasm-opt

Recommended:

npm run watch-dashboard
./run ide watch --skip-version-check --skip-wasm-opt --backend-source release --backend-release latest
- and with --ide-option -authentication
./run ide build --skip-version-check --skip-wasm-opt --backend-source release --backend-release latest
- Enso and Enso -authentication

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.


          Fix gui watch; remove copy plugin; various minor esbuild-related fixes

07cc373

somebody1234 requested review from MichaelMauderer, wdanilo and kazcw as code owners

April 5, 2023 18:01

somebody1234 added the CI: No changelog needed label

Contributor

PabloBuchu commented Apr 5, 2023

QA 🟢

Confrim that ./run gui watch works and reloads itself after file changes.
Build works (authentication + ide + workspace).
watch-dashboard & authentication also no issues

Contributor

PabloBuchu commented Apr 6, 2023 •

edited by unfurl-links bot

@somebody1234 can you take a look on windows building? its failing https://github.com/enso-org/enso/actions/runs/4621430061/jobs/8182386985?pr=6212
maybe @Nctdt could help you since he is working on windows

GitHub
Fix `./run gui watch` · enso-org/enso@07cc373
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

PabloBuchu mentioned this pull request

Add cloud endpoints for frontend #6002

Merged

5 tasks

Collaborator Author

somebody1234 commented Apr 6, 2023

ah whoops, it's because i'm using a hardcoded regex to detect folders and forgot to factor in windows


          Fix copy-directories plugin on Windows

7fa6aa8

Collaborator Author

somebody1234 commented Apr 6, 2023 •

edited

this should fix it but it'd be good to get extra testing to make sure i didn't accidentally break it for macos
(i guess CI will error if it's broken though)

Contributor

PabloBuchu commented Apr 6, 2023

@somebody1234 build went ok on ma laptop

somebody1234 mentioned this pull request

Add dashboard table view #6015

Merged

6 tasks

Collaborator Author

somebody1234 commented Apr 6, 2023

@wdanilo just a heads up - the fix for gui watch is here and has been QA'd. you mentioned that this should be fixed ASAP so you might want to take a look at this sometime soon?

Contributor

PabloBuchu commented Apr 7, 2023

@mwu-tow are you able to do review for this PR? it fixes problem with ./run gui watch not able to reload browser page after code change

mwu-tow reviewed

View reviewed changes

app/ide-desktop/lib/content/esbuild-config.ts Outdated

                       outdir: outputPath,
                       outbase: 'src',
                       plugins: [
+                          {
+                              // This is a workaround that is needed because esbuild panics when using `loader: { '.js': 'copy' }`.

Contributor

mwu-tow Apr 7, 2023

Why does it panic? Is this a bug in esbuild? Was it reported?

Collaborator Author

somebody1234 Apr 7, 2023

reported here. it's a triggered by bundling ts and using js: copy and inject all at the same time

Collaborator Author

somebody1234 Apr 10, 2023

(the bug has been fixed!! once the next release is made, we should be back to no inline plugins for this package)

app/ide-desktop/lib/content/esbuild-config.ts

+                              // This is a workaround that is needed because esbuild panics when using `loader: { '.js': 'copy' }`.
+                              name: 'pkg-js-is-cjs',
+                              setup: build => {
+                                  build.onLoad({ filter: /\/pkg.js$/ }, async ({ path }) => ({

Contributor

mwu-tow Apr 7, 2023

Why do we add a special rule just for pkg.js? Please document, how pkg.js is special.

Collaborator Author

somebody1234 Apr 7, 2023

will do.
(explaining here too for posterity:)
esbuild converts files to ESM because the package.json says so, however pkg.js is loaded using new Function in wasm-pack code, i believe - so the default output (pkg.js being a module) throws a syntax error

app/ide-desktop/lib/content/esbuild-config.ts

                       outdir: outputPath,
                       outbase: 'src',
                       plugins: [
+                          {
+                              // This is a workaround that is needed because esbuild panics when using `loader: { '.js': 'copy' }`.
+                              name: 'pkg-js-is-cjs',

Contributor

mwu-tow Apr 7, 2023

Why does setting loader to copy mean the file "is cjs"?

Collaborator Author

somebody1234 Apr 7, 2023

will add comment - loader: copy prevents esbuild from processing the file, otherwise it becomes ESM which breaks the way we load the file

app/ide-desktop/lib/content/esbuild-config.ts Outdated

+                                  }))
+                              },
+                          },
+                          {

Contributor

mwu-tow Apr 7, 2023

I'm not sure about inlining such things. While it is not necessary to keep this as a separate package, it should be extracted, documented and be reusable, e.g. if "client" bundler ever wants to copy directories.

app/ide-desktop/lib/content/esbuild-config.ts Outdated

+                              setup: build => {
+                                  build.onResolve({ filter: /[/\\][^./\\]+$/ }, ({ path, kind }) =>
+                                      kind === 'entry-point'
+                                          ? { path, namespace: 'copy-directories', watchDirs: [path] }

Contributor

mwu-tow Apr 7, 2023

I don't think it will update when the files content change, watchDir is only for the entry list changes. Also, it does not watch subdirectories.

Collaborator Author

somebody1234 Apr 7, 2023

this is true, however afaict the same is true for the old copy plugin - since directories are copied via fs.cp(from, to, { recursive: true }) and then pushed to watchFiles. since these are resolved on every build (i think) because they are entry points, they are probably always copied - i can add a comment after extracting out to a plugin

Collaborator Author

somebody1234 Apr 7, 2023

wait actually... it still won't update when file contents change. we might need to use chokidar and manually trigger rebuilds? alternatively leave as-is, but with a comment noting this as a weakness of this approach

Collaborator Author

somebody1234 Apr 7, 2023

oh, alternatively chokidar can be used to continuously keep the directories in sync after the initial cp, skipping all onResolve and onLoad except for the first onResolve, which would register the watcher

app/ide-desktop/lib/content/src/index.ts Outdated

+              // ===================
+              if (IS_DEV_MODE) {
+                  new EventSource('/esbuild').addEventListener('change', () => {

Contributor

mwu-tow Apr 7, 2023

The '/esbuild' and 'change' look here like magic literals. Please document.
(BTW, it's great to see this finally supported in esbuild itself.)

Contributor

mwu-tow commented Apr 7, 2023

@PabloBuchu I did, but you'll still need a code owner's review.


          Address review issues

ef2fe39

Collaborator Author

somebody1234 commented Apr 7, 2023

took a bit longer than expected but review issues should be addressed now.

somebody1234 added 3 commits

April 8, 2023 04:49


          Minor formatting fix

667ca9f


          Run prettier

50ce6c8


          Merge branch 'develop' into wip/sb/fix-gui-watch

7bf567d

wdanilo approved these changes

View reviewed changes

PabloBuchu added the CI: Ready to merge label


          Merge branch 'develop' into wip/sb/fix-gui-watch

4dea6e7

Collaborator Author

somebody1234 commented Apr 11, 2023 •

edited

ah, the issue is that chokidar continues running indefinitely even after esbuild stops...
not sure if there's a way to detect when esbuild is finished though...

Collaborator Author

somebody1234 commented Apr 11, 2023

ah, this

somebody1234 added 2 commits

April 11, 2023 14:59


          Fix chokidar watchers hanging around indefinitely

a8d668d


          Minor fix

Collaborator Author

somebody1234 commented Apr 11, 2023

well that didn't fix it

somebody1234 added 2 commits

April 11, 2023 15:14


          Actually fix the hang

8d1cc60


          Minor documentation change

ebbd610

mergify bot merged commit 37d820c into develop

24 checks passed

mergify bot deleted the wip/sb/fix-gui-watch branch

April 11, 2023 06:04

MichaelMauderer pushed a commit that referenced this pull request


          Fix ./run gui watch (#6212)

9ee8e97

- Fixes #6168
- Removes `enso-copy-plugin` in favor of an inline plugin
- It was only used in one place anyway
- It is probably necessary since I've "fixed" it by adding all files as entrypoints (I'm not quite sure why it wasn't working with the fix with `enso-copy-plugin`...)
- Adds live reload (back) to `content/`

# Important Notes
To QA:
Mandatory:
- `./run gui watch --skip-version-check --skip-wasm-opt`

Recommended:
- `npm run watch-dashboard`
- `./run ide watch --skip-version-check --skip-wasm-opt --backend-source release --backend-release latest`
- and with `--ide-option -authentication`
- `./run ide build --skip-version-check --skip-wasm-opt --backend-source release --backend-release latest`
- `Enso` and `Enso -authentication`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment