Using esbuild instead of rollup #1298

WebReflection · 2023-03-20T16:39:58Z

Description

This PR migrates building tasks to esbuild so that it takes ~100ms instead of 6 seconds to create our artifacts.

Changes

removed all rollup dependencies and related files
created an esmbuild config file that mimics what rollup was doing before
added a python server as I think we all have python installed :-)
added a node watch for the ./src folder that kicks in esbuild when changes happen
added a make build-fast entry in Makefile and removed npm run build-min form the package.json

Checklist

All tests pass locally
I have updated docs/changelog.md
I have created documentation for this(if applicable)

antocuni

This PR mostly LGTM, although I don't know enough about JS tooling to make an informed call: but if you are confident that the end result is the same as before, I'm happy with it.

However, I think there is a problem: I tried to deliberately put some mistakes in the .ts files but the build doesn't complain.

E.g., I tried to apply this diff:

diff --git a/pyscriptjs/src/main.ts b/pyscriptjs/src/main.ts
index cd442cb..73d30ec 100644
--- a/pyscriptjs/src/main.ts
+++ b/pyscriptjs/src/main.ts
@@ -59,7 +59,7 @@ More concretely:
 */
 
 export class PyScriptApp {
-    config: AppConfig;
+    //config: AppConfig;
     interpreter: InterpreterClient;
     PyScript: ReturnType<typeof make_PyScript>;
     plugins: PluginManager;
@@ -168,9 +168,12 @@ export class PyScriptApp {
     // point (4) to point (5).
     //
     // Invariant: this.config is set and available.
-    async afterInterpreterLoad(interpreter: InterpreterClient): Promise<void> {
+    async afterInterpreterLoad(interpreter: number): Promise<void> {
         console.assert(this.config !== undefined);
 
+        this.I_dont_exist();
+        aaa();
+
         this.logStatus('Python startup...');
         await this.interpreter.initializeRemote();
         this.logStatus('Python ready!');

But npm run build doesn't complain:

$ npm run build

> pyscript@0.0.1 build
> node esbuild.mjs

pyscript (prod) built in: 132.779ms

pyscriptjs/esbuild.mjs

WebReflection · 2023-03-21T18:16:01Z

@antocuni I believe esbuild doesn't bother with TypeScript/JS possible errors, likely the reason is faster than others. We have all hooks and linters and prettifiers and tools ahead of commits, which I think (correct me if I am wrong) would already prevent shenanigans down the CI pipe. My thinking here is that if we are defensive on pre-hooks, CI, linting, TS-test/check, prettifier, and whatever else, we shouldn't really demand the bundler to be in charge of all these early errors already visible and caught before the bundler even run ... so we have two options:

delegate code validation to the minifier/bundler and drop everything else before
keep it simple and trust the workflow pipe will already alert us if something is wrong in the code

I am more into the second option kind of developer, but I understand where you come from, although minification in isolation is to me a step too late to expect warnings around the correctness of the code ... we have other tools for that.

WebReflection · 2023-03-22T10:20:16Z

@antocuni as discussed in discord I've put npm run tsc in front of esbuild as main npm run build command which is what the Makefile uses too. I hope this works well for you, let me know if that's not the case.

JeffersGlass · 2023-03-22T13:05:55Z

Perhaps this is is an issue in my environment, but neither make dev nor npm run dev are currently working for me:

make dev

$ make dev
npm run dev

> pyscript@0.0.1 dev
> NODE_WATCH=1 node esbuild.mjs

pyscript (dev) built in: 70.724ms
node:internal/errors:464
    ErrorCaptureStackTrace(err);
    ^

TypeError [ERR_FEATURE_UNAVAILABLE_ON_PLATFORM]: The feature watch recursively is unavailable on the current platform, which is being used to run Node.js
    at new NodeError (node:internal/errors:371:5)
    at watch (node:fs:2245:11)
    at file:///home/jglass/Documents/pyscript/pyscriptjs/esbuild.mjs:60:5 {
  code: 'ERR_FEATURE_UNAVAILABLE_ON_PLATFORM'
}
make: *** [Makefile:56: dev] Error 1

This stackoverflow post implies this is an issue with node v14+ (I happen to have v16.13.1); some fixes also proposed there.

npm run dev

$ npm run dev

> pyscript@0.0.1 dev
> NODE_WATCH=1 node esbuild.mjs

node:internal/process/esm_loader:94
    internalBinding('errors').triggerUncaughtException(
                              ^

[Error: ENOENT: no such file or directory, copyfile 'build/index.html' -> 'examples/build/index.html'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'copyfile',
  path: 'build/index.html',
  dest: 'examples/build/index.html'
}

Not sure what's going on here - I've tried running npm run dev from various directories, to not effect. The file at pyscriptjs/build/index.html does exist. This is a bash/ubuntu setup, for what it's worth.

JeffersGlass · 2023-03-22T13:15:23Z

From the esbuild typescript recommendations, I'd proposed we add "isolatedModules": true, to tsconfig.json, so that our typescript compilation is consistent with how esbuild is parsing/compiling each file.

There seem to be only a few patterns this precludes, none of which we use, like re-exporting an imported type.

I should say - this is really awesome work, and I'm very much looking forward to the build speed improvements!

tedpatrick · 2023-03-22T13:25:54Z

I love ESBuild, this is worth doing. 👍

WebReflection · 2023-03-22T13:29:18Z

@JeffersGlass I think the recursive issue is due nodejs LTS not having that ability ... shame on me I've tested on node 19.

The second issue might be due usage of import.meta.url also not available in older LTS ... I might just use CommonJS here to simplify everyone life and backward compatibility, if that's OK.

WebReflection · 2023-03-22T14:06:24Z

@JeffersGlass I've moved to CommonJS to be sure the path is well enforced everywhere without using latest ESM features but the cp utility exists since Node.JS v16 LTS while the "soon to be defunct" LTS 14 doesn't have it.

I have also created a recursive function to watch recursively so no error should be shown there.

Am I assuming correctly LTS 16 is the minimum version of Node? Can you please verify everything is working now? Thanks!

antocuni

LGTM!
FWIW, npm run dev works on my machine with node 18.5.0.

antocuni · 2023-03-22T15:10:20Z

@WebReflection I think this is ready to be merged, unless you have other edits to do. I'll merge it as soon as the build is green (ping me if I forget 😅)

JeffersGlass · 2023-03-22T15:20:01Z

So make dev now works... but for some unfathomable reason npm run dev does not?

UPDATE - it does work, but needed to run npm run build first. Something I wouldn't have necessarily expected.

SECOND UPDATE - See below.

(/home/jglass/Documents/pyscript/pyscriptjs/env) jglass@jglass-XPS-8930:~/Documents/pyscript/pyscriptjs$ npm run dev

> pyscript@0.0.1 dev
> NODE_WATCH=1 node esbuild.js

node:internal/process/promises:246
          triggerUncaughtException(err, true /* fromPromise */);
          ^

[Error: ENOENT: no such file or directory, copyfile '/home/jglass/Documents/pyscript/pyscriptjs/build/index.html' -> '/home/jglass/Documents/pyscript/pyscriptjs/examples/build/index.html'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'copyfile',
  path: '/home/jglass/Documents/pyscript/pyscriptjs/build/index.html',
  dest: '/home/jglass/Documents/pyscript/pyscriptjs/examples/build/index.html'
}

JeffersGlass · 2023-03-22T15:24:50Z

Second Update: both make dev and npm run dev sometimes work, sometimes do not, and sometimes surprisingly fail in the middle of a build.

Check out the terminal copy below - this is just adding some extraneous spaces to a single file and resaving. It rebuilt successfully twice, then crashed on copy.

Like I say, I hope this is just a me-problem, but I am baffled.

(/home/jglass/Documents/pyscript/pyscriptjs/env) jglass@jglass-XPS-8930:~/Documents/pyscript/pyscriptjs$ npm run dev

> pyscript@0.0.1 dev
> NODE_WATCH=1 node esbuild.js

pyscript (dev) built in: 94.102ms
Serving HTTP on 0.0.0.0 port 8080 (http://0.0.0.0:8080/) ...
pyscript (dev) built in: 69.97ms
node:internal/process/promises:246
          triggerUncaughtException(err, true /* fromPromise */);
          ^

[Error: ENOENT: no such file or directory, copyfile '/home/jglass/Documents/pyscript/pyscriptjs/build/index.html' -> '/home/jglass/Documents/pyscript/pyscriptjs/examples/build/index.html'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'copyfile',
  path: '/home/jglass/Documents/pyscript/pyscriptjs/build/index.html',
  dest: '/home/jglass/Documents/pyscript/pyscriptjs/examples/build/index.html'
}

WebReflection · 2023-03-22T15:33:29Z

@JeffersGlass

I am afraid I am very confused here ...

So make dev now works... but for some unfathomable reason npm run dev does not?
UPDATE - it does work, but needed to run npm run build first.

make dev command runs exactly npm run dev and nothing else and npm run dev runs exactly what happens in npm run build ... do you mind sharing which version of node are you using?

The CI is using 18 LTS but I've tested in 16 LTS and I can never reproduce what you are seeing ... which makes me think that this is the case:

I hope this is just a me-problem

but I'd love to get to the bottom of this.

edit FYI if you are on 14 LTS it's almost at its end of life https://nodejs.dev/en/about/releases/

WebReflection · 2023-03-22T15:35:34Z

@antocuni after all it looks like the src/plugins/python/ folder had to be moved around or integration fails ... this last push should address that so please wait to merge, thanks and sorry for removing too much form the leftover.

antocuni · 2023-03-22T15:41:24Z

Second Update: both make dev and npm run dev sometimes work, sometimes do not, and sometimes surprisingly fail in the middle of a build.

this is a bit worrisome. I wonder whether it has something to do with your IDE or with esbuild itself.
I did this test:

in one terminal, make dev
in another terminal:

$ cd pyscriptjs/src
$ for i in `seq 1 10`; do echo $i; touch main.ts; sleep 3; done

This forces the rebuild 10 times in a row. On my machine, it works reliably:

$ npm run dev

> pyscript@0.0.1 dev
> NODE_WATCH=1 node esbuild.js

pyscript (dev) built in: 118.548ms
Serving HTTP on 0.0.0.0 port 8080 (http://0.0.0.0:8080/) ...
pyscript (dev) built in: 132.773ms
pyscript (dev) built in: 130.566ms
pyscript (dev) built in: 130.516ms
pyscript (dev) built in: 120.426ms
pyscript (dev) built in: 108.242ms
pyscript (dev) built in: 141.805ms
pyscript (dev) built in: 139.569ms

@JeffersGlass what is the file that you are editing? Is it the same file that esbuild complains about or it's another one? Does esbuild always complain about index.html, or the file that it can't find is random?
Could you please try the experiment above and see whether it works?

antocuni · 2023-03-22T15:42:24Z

@antocuni after all it looks like the src/plugins/python/ folder had to be moved around or integration fails ...

yes, those are needed for sure because they are fetched/imported by pyodide at runtime. Sorry for not being clear about that.

JeffersGlass · 2023-03-22T15:53:27Z

make dev command runs exactly npm run dev and nothing else and npm run dev runs exactly what happens in npm run build ...

I suspect my initial impression was wrong - make dev was appearing to work, but has the same intermitant failures for me that npm run dev does; make dev just happened to work for a bit the first time I ran it.

Cloning this branch fresh, I am unable to reproduce my issue. That is to say, the following steps/commands leaves me unable to reproduce my earlier issue, and additionally, running your shell loop builds successfully many times in a row.

Clone pyscript/pyscript.git to new folder
git remote add WR https://github.com/WebReflection/pyscript.git
git switch esbuild
cd pyscriptjs
make setup
make dev
	npm run dev

	> pyscript@0.0.1 dev
	> NODE_WATCH=1 node esbuild.js

	pyscript (dev) built in: 78.706ms
	Serving HTTP on 0.0.0.0 port 8080 (http://0.0.0.0:8080/) ...

Running your test on my original copy fails immediately the first time the loop touches main.ts. It is complaining about "no such file or directory" at "pyscriptjs/build/index.html", though that file does exist on my machine. Same issue as copied above:

node:internal/process/promises:246
          triggerUncaughtException(err, true /* fromPromise */);
          ^

[Error: ENOENT: no such file or directory, copyfile '/home/jglass/Documents/pyscript/pyscriptjs/build/index.html' -> '/home/jglass/Documents/pyscript/pyscriptjs/examples/build/index.html'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'copyfile',
  path: '/home/jglass/Documents/pyscript/pyscriptjs/build/index.html',
  dest: '/home/jglass/Documents/pyscript/pyscriptjs/examples/build/index.html'

So at least it's not a reproducible issue in a fresh copy; almost certainly something odd happening in my environment.

antocuni · 2023-03-22T15:56:27Z

So at least it's not a reproducible issue in a fresh copy; almost certainly something odd happening in my environment.

good, this mean that we can proceed with the merge. At the same time, the nerd which is inside me would be really curious to know what is causing the issue in your old environment. Probably not worth spending time on it, though :)

Cool, so I'll proceed an merge this!

WebReflection changed the title ~~WIP: Using esbuild instead of rollup~~ Using esbuild instead of rollup Mar 21, 2023

antocuni requested changes Mar 21, 2023

View reviewed changes

pyscriptjs/esbuild.mjs Outdated Show resolved Hide resolved

pyscriptjs/esbuild.mjs Outdated Show resolved Hide resolved

WebReflection mentioned this pull request Mar 22, 2023

Outdated dist/index.html should be fixed #1302

Closed

3 tasks

antocuni approved these changes Mar 22, 2023

View reviewed changes

Using esbuild instead of rollup

2d39809

antocuni merged commit 51d5140 into pyscript:main Mar 22, 2023

hoodmane mentioned this pull request Mar 23, 2023

Allow pyscript package to contain multiple files #1262

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using esbuild instead of rollup #1298

Using esbuild instead of rollup #1298

WebReflection commented Mar 20, 2023 •

edited

Loading

antocuni left a comment

WebReflection commented Mar 21, 2023 •

edited

Loading

WebReflection commented Mar 22, 2023

JeffersGlass commented Mar 22, 2023 •

edited

Loading

JeffersGlass commented Mar 22, 2023 •

edited

Loading

tedpatrick commented Mar 22, 2023

WebReflection commented Mar 22, 2023

WebReflection commented Mar 22, 2023 •

edited

Loading

antocuni left a comment

antocuni commented Mar 22, 2023

JeffersGlass commented Mar 22, 2023 •

edited

Loading

JeffersGlass commented Mar 22, 2023

WebReflection commented Mar 22, 2023 •

edited

Loading

WebReflection commented Mar 22, 2023

antocuni commented Mar 22, 2023

antocuni commented Mar 22, 2023

JeffersGlass commented Mar 22, 2023 •

edited

Loading

antocuni commented Mar 22, 2023

Using esbuild instead of rollup #1298

Using esbuild instead of rollup #1298

Conversation

WebReflection commented Mar 20, 2023 • edited Loading

Description

Changes

Checklist

antocuni left a comment

Choose a reason for hiding this comment

WebReflection commented Mar 21, 2023 • edited Loading

WebReflection commented Mar 22, 2023

JeffersGlass commented Mar 22, 2023 • edited Loading

make dev

npm run dev

JeffersGlass commented Mar 22, 2023 • edited Loading

tedpatrick commented Mar 22, 2023

WebReflection commented Mar 22, 2023

WebReflection commented Mar 22, 2023 • edited Loading

antocuni left a comment

Choose a reason for hiding this comment

antocuni commented Mar 22, 2023

JeffersGlass commented Mar 22, 2023 • edited Loading

JeffersGlass commented Mar 22, 2023

WebReflection commented Mar 22, 2023 • edited Loading

WebReflection commented Mar 22, 2023

antocuni commented Mar 22, 2023

antocuni commented Mar 22, 2023

JeffersGlass commented Mar 22, 2023 • edited Loading

antocuni commented Mar 22, 2023

WebReflection commented Mar 20, 2023 •

edited

Loading

WebReflection commented Mar 21, 2023 •

edited

Loading

JeffersGlass commented Mar 22, 2023 •

edited

Loading

JeffersGlass commented Mar 22, 2023 •

edited

Loading

WebReflection commented Mar 22, 2023 •

edited

Loading

JeffersGlass commented Mar 22, 2023 •

edited

Loading

WebReflection commented Mar 22, 2023 •

edited

Loading

JeffersGlass commented Mar 22, 2023 •

edited

Loading