-
Notifications
You must be signed in to change notification settings - Fork 316
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 ./run gui watch
#6212
Fix ./run gui watch
#6212
Conversation
QA 🟢
|
@somebody1234 can you take a look on windows building? its failing https://github.com/enso-org/enso/actions/runs/4621430061/jobs/8182386985?pr=6212
|
ah whoops, it's because i'm using a hardcoded regex to detect folders and forgot to factor in windows |
this should fix it but it'd be good to get extra testing to make sure i didn't accidentally break it for macos |
@somebody1234 build went ok on ma laptop |
@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? |
@mwu-tow are you able to do review for this PR? it fixes problem with |
outdir: outputPath, | ||
outbase: 'src', | ||
plugins: [ | ||
{ | ||
// This is a workaround that is needed because esbuild panics when using `loader: { '.js': 'copy' }`. |
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.
Why does it panic? Is this a bug in esbuild? Was it reported?
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.
reported here. it's a triggered by bundling ts
and using js: copy
and inject
all at the same time
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.
(the bug has been fixed!! once the next release is made, we should be back to no inline plugins for this package)
// 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 }) => ({ |
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.
Why do we add a special rule just for pkg.js
? Please document, how pkg.js
is special.
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.
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
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', |
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.
Why does setting loader to copy mean the file "is cjs"?
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.
will add comment - loader: copy
prevents esbuild from processing the file, otherwise it becomes ESM which breaks the way we load the file
})) | ||
}, | ||
}, | ||
{ |
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.
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.
setup: build => { | ||
build.onResolve({ filter: /[/\\][^./\\]+$/ }, ({ path, kind }) => | ||
kind === 'entry-point' | ||
? { path, namespace: 'copy-directories', watchDirs: [path] } |
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.
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.
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.
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
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.
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
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.
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
// =================== | ||
|
||
if (IS_DEV_MODE) { | ||
new EventSource('/esbuild').addEventListener('change', () => { |
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.
The '/esbuild'
and 'change'
look here like magic literals. Please document.
(BTW, it's great to see this finally supported in esbuild itself.)
@PabloBuchu I did, but you'll still need a code owner's review. |
took a bit longer than expected but review issues should be addressed now. |
ah, the issue is that |
ah, this |
well that didn't fix it |
- 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`
Pull Request Description
enso-copy-plugin
in favor of an inline pluginenso-copy-plugin
...)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
--ide-option -authentication
./run ide build --skip-version-check --skip-wasm-opt --backend-source release --backend-release latest
Enso
andEnso -authentication
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build
.