scripts: add `s` script (as a faster `start` option), allowing very fast demo building when developing #1003

peaBerberian · 2021-09-09T17:04:31Z

What this PR does

This PR tries to improve demo building time by using the notoriously faster esbuild bundler instead of the Webpack bundler we use by default.

I also profited from this PR to improve on the current webpack config scripts by making them configurable through command line arguments instead of environment variables. This should be easier to understand and follow. That part can (and most probably should!) be added in a separate PR.

By switching from webpack to esbuild when starting a demo, we go on my laptop from 15s-16s to 100-200ms to build the demo page.
16 seconds was not much, but it was enough that it could lead me to switching to another task, waiting for the demo to be built - and thus usually costed much more than that. I would not be surprised if I was the only one in that situation.
With sub-second results, a code modification can be tested instantly.

By making demo bundling close-to costless, it could also improve the time of the bisect operations we sometimes (arguably very rarely) perform and it allows nice new usages like testing multiple demo versions easily.

We could go even further by using something like open to open the developer's default browser on the demo page directly (of course more advanced testing will need to test on multiple browsers, but we're talking about fast checks here).

Because I did not want to take the bet - and because there are some disadvantages in using esbuild - I still left Webpack by default when running the npm run start script. esbuild is only used when using the npm run s.

Webpack is also still used when generating the demos for production, the tests and the legacy builds (dist/rx-player.js and dist/rx-player.min.js). Switching to esbuild for those could be possible in the future, but Webpack is not a pain point in those cases for now (considering tests are long anyway and the others are rare occurrences).

Disadvantages

No type-checking performed by esbuild

Among the disadvantages in using esbuild, the main one I found is that unlike Webpack, esbuild cannot be used to type-check the files while they are bundled.

At first, I tried to write a script performing both in parallel (bundling and type-checking) to emulate the current npm run start behavior, but as it turns out TypeScript did multiple things that made this experiment a pain:

TypeScript's JavaScript API is both unstable and incomplete, so I could not just write a simple Node.js script handling its output in a simple and straightforward way like we do for webpack
tsc binary's watch option - which I wanted to use here - clears the screen and adds many line breaks by default, making it harder to making it cohabit with a second script displaying on the same output.

So for now, I only added a supplementary npm script, npm run check:types:watch which performs type-checking while watching the files it type-checks (something we might need anyway).

It is a supplementary dependency

Another thing I do not like is that it is a supplementary dependency which moreover is totally optional.
Thus it adds maintenance cost and also 7MB to the project which leads to approximately a 3% size increase of the node_modules directory.

This is not much at all but I still tried to make that dependency "opt-outable" when npm installing by e.g. making it an optional dependency.
However, turns out only optionalDependencies exist, and not optionalDevDependencies like we would want.

An old issue seems to have been created about that on npm's issue tracker, npm/npm#3870, but the conclusion seems to be that this is both complicated to write for them and not an important enough feature.

But in truth, I recognize that this is kind of an overengineering need.
3% increase of the node_modules directory is nothing and this dependency only has an impact on locally built demo.

What do you think?

Do you think reducing demo building time is even a real need (I won't be angry if you don't :D)?

peaBerberian · 2021-09-09T17:06:24Z

demo/full/scripts/controllers/Main.jsx

@@ -1,4 +1,4 @@
-import RxPlayer from "rx-player";
+import RxPlayer from "../../../../src/index.ts";


esbuild doesn't do alias directly. It relies on jsconfig.json (apparently a VSCode config file!) and tsconfig.json files that can define it for it.

However I did not want to add yet another config file at the root for what is moreover only a demo need.
So I went back to relative imports, which are exactly the same thing.

peaBerberian · 2021-09-09T17:07:29Z

scripts/generate_full_demo.js

 */
 function generateFullDemo(options) {
+  const shouldMinify = options.minify === true;


I moved webpack-demo.config.js here

peaBerberian · 2021-09-09T17:07:56Z

tests/generate_test_webpack_config.js

+ * taken to allow code coverage reports.
+ * @returns {Object} - The Webpack config
+ */
+module.exports = function generateTestWebpackConfig({


I moved webpack-tests.config.js here

…ry wording

PaulRosset · 2021-10-25T13:13:05Z

package.json

@@ -54,6 +54,7 @@
    "prepublishOnly": "npm run build:modular",
    "standalone": "node ./scripts/run_standalone_demo.js",
    "start": "node ./scripts/start_demo_web_server.js",
+    "s": "node ./scripts/start_demo_web_server.js --fast",


There is any reason why it is named s? Make it harder to reason about what about start-fast?

Yes I updated it because if the main point of this new script is to perform very quick checks, it made more sense to me that the script's name was not longer to type.

PaulRosset · 2021-10-25T13:14:21Z

scripts/start_demo_web_server.js

+              minify: false,
+              production: false });
+} else {
+  slowBuild({ watch: true,


This is something very minor but I guess the script is not slow this is just esbuild that kill the competition :)

yeah it's to create a clear dichotomy with the "fast" one!

PaulRosset · 2021-10-25T13:18:55Z

scripts/fast_demo_build.js

+  const minify = !!options.minify;
+  const watch = !!options.watch;
+  const isDevMode = !options.production;
+  let beforeTime = performance.now();


It seems that nodejs need that dep to use performance:

https://stackoverflow.com/a/46502819/9098915

https://nodejs.org/api/perf_hooks.html#performancenow

Cause I got that issue on Nodejs v14.15.1

I think it might depends on the nodejs version yes.
On mine it is available without importing perf-hooks.

But more interestingly, the verified answer from your stack overflow, talk about process.hrtime. However, the nodejs documentation isn't explicit on if this is a monotonic clock :/
I will check. At worst, it is only used for time indication to the user, no biggie if it isn't that coherent when the clock is updated !

PaulRosset · 2021-10-25T13:21:33Z

I see a real use case and a real benefit for developers working every day on the project.

peaBerberian added script Relative to the RxPlayer's scripts (e.g. build scripts, release scripts etc.) dependencies Pull requests that update a dependency file labels Sep 9, 2021

peaBerberian commented Sep 9, 2021

View reviewed changes

peaBerberian force-pushed the misc/esbuild branch from 6b0de0d to 811bd88 Compare September 10, 2021 11:56

peaBerberian mentioned this pull request Sep 10, 2021

build: remove the need to use environment variable when building the player, demo or tests #1004

Merged

peaBerberian force-pushed the misc/esbuild branch 2 times, most recently from e60c533 to cbf7232 Compare September 15, 2021 12:50

peaBerberian changed the title ~~scripts: add start:fast script, allowing very fast demo building when developing~~ scripts: add s script (as a faster start option), allowing very fast demo building when developing Oct 15, 2021

peaBerberian added 2 commits October 15, 2021 14:40

scripts: add start:fast script, allowing very fast demo building

c501fba

scripts: rename start:fast script into just s. Simplify its explanato…

d72cefc

…ry wording

peaBerberian force-pushed the misc/esbuild branch from 44dd9fd to d72cefc Compare October 15, 2021 12:40

PaulRosset reviewed Oct 25, 2021

View reviewed changes

scripts: stop relying on performance.now in node scripts

ea725d8

peaBerberian merged commit fe71ab4 into master Nov 4, 2021

peaBerberian added this to the 3.26.2 milestone Nov 9, 2021

peaBerberian deleted the misc/esbuild branch December 3, 2021 15:36

peaBerberian mentioned this pull request Dec 16, 2021

Release/v3.26.2 #1034

Merged

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scripts: add `s` script (as a faster `start` option), allowing very fast demo building when developing #1003

scripts: add `s` script (as a faster `start` option), allowing very fast demo building when developing #1003

peaBerberian commented Sep 9, 2021 •

edited

Loading

peaBerberian Sep 9, 2021 •

edited

Loading

peaBerberian Sep 9, 2021

peaBerberian Sep 9, 2021 •

edited

Loading

PaulRosset Oct 25, 2021

peaBerberian Oct 25, 2021

PaulRosset Oct 25, 2021 •

edited

Loading

peaBerberian Oct 25, 2021

PaulRosset Oct 25, 2021 •

edited

Loading

peaBerberian Oct 25, 2021

peaBerberian Oct 26, 2021

PaulRosset commented Oct 25, 2021

		@@ -1,4 +1,4 @@
		import RxPlayer from "rx-player";
		import RxPlayer from "../../../../src/index.ts";

scripts: add s script (as a faster start option), allowing very fast demo building when developing #1003

scripts: add s script (as a faster start option), allowing very fast demo building when developing #1003

Conversation

peaBerberian commented Sep 9, 2021 • edited Loading

What this PR does

Disadvantages

No type-checking performed by esbuild

It is a supplementary dependency

peaBerberian Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

peaBerberian Sep 9, 2021

Choose a reason for hiding this comment

peaBerberian Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

PaulRosset Oct 25, 2021

Choose a reason for hiding this comment

peaBerberian Oct 25, 2021

Choose a reason for hiding this comment

PaulRosset Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

peaBerberian Oct 25, 2021

Choose a reason for hiding this comment

PaulRosset Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

peaBerberian Oct 25, 2021

Choose a reason for hiding this comment

peaBerberian Oct 26, 2021

Choose a reason for hiding this comment

PaulRosset commented Oct 25, 2021

scripts: add `s` script (as a faster `start` option), allowing very fast demo building when developing #1003

scripts: add `s` script (as a faster `start` option), allowing very fast demo building when developing #1003

peaBerberian commented Sep 9, 2021 •

edited

Loading

peaBerberian Sep 9, 2021 •

edited

Loading

peaBerberian Sep 9, 2021 •

edited

Loading

PaulRosset Oct 25, 2021 •

edited

Loading

PaulRosset Oct 25, 2021 •

edited

Loading