Skip to content
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

[api-major] Output JavaScript modules in the builds (issue 10317) #17055

Merged
merged 3 commits into from
Oct 7, 2023

Conversation

Snuffleupagus
Copy link
Collaborator

At this point in time all browsers, and also Node.js, support standard import/export statements and we can now finally consider outputting modern JavaScript modules in the builds.[1]

In order for this to work we can only use proper import/export statements throughout the main code-base, and (as expected) our Node.js support made this much more complicated since both the official builds and the GitHub Actions-based tests must keep working.[2]
One remaining issue is that the pdf.scripting.js file cannot be built as a JavaScript module, since doing so breaks PDF scripting. It's not clear to me if this is a limitation of the QuickJS Javascript Engine itself, or "just" an issue with how the Emscripten compiler was configured.

Note that my initial goal was to try and split these changes into a couple of commits, however that unfortunately didn't really work since it turned out to be difficult for smaller patches to work correctly and pass (all) tests that way.[3]
This is a classic case of every change requiring a couple of other changes, with each of those changes requiring further changes in turn and the size/scope quickly increasing as a result.

One possible "issue" with these changes is that we'll now only output JavaScript modules in the builds, which could perhaps be a problem with older tools. However it unfortunately seems far too complicated/time-consuming for us to attempt to support both the old and modern module formats, hence the alternative would be to do "nothing" here and just keep our "old" builds.[4]


[1] The final blocker was module support in workers in Firefox, which was implemented in Firefox 114; please see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import#browser_compatibility

[2] It's probably possible to further improve/simplify especially the Node.js-specific code, but it does appear to work as-is.

[3] Having partially "broken" patches, that fail tests, as part of the commit history is really not a good idea in general.

[4] Outputting JavaScript modules was first requested almost five years ago, see issue #10317, and nowadays there should be much better support for JavaScript modules in various tools.

@Snuffleupagus Snuffleupagus linked an issue Oct 2, 2023 that may be closed by this pull request
@Snuffleupagus Snuffleupagus force-pushed the output-modules branch 3 times, most recently from b856022 to 469789e Compare October 2, 2023 10:39
@Snuffleupagus
Copy link
Collaborator Author

Given the errors when loading the previews above, my guess would be that the server running on the bots doesn't understand the .mjs file extension. Note that for our local development server we had to add an entry as follows:

".mjs": "application/javascript",

@calixteman Can you please check if this can be fixed on the bots?

@calixteman
Copy link
Contributor

/botio-linux preview

@Snuffleupagus Snuffleupagus force-pushed the output-modules branch 2 times, most recently from acf25b4 to 9c8232b Compare October 2, 2023 12:08
@mozilla mozilla deleted a comment from moz-tools-bot Oct 2, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Oct 2, 2023
@calixteman
Copy link
Contributor

So it works but I had to modify botio/node_modules/mime/types/mime.types in order to explicitly add the correct mime type for mjs files.
I've the feeling that it's an awful way to fix the issue but I'm not sure I want to take the risk to break something in doing it correctly. Wdyt ?

@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Oct 2, 2023

What version of the mime package, see https://www.npmjs.com/package/mime, are the bots using?
Could it just be very old and outdated, and if so updating it to the latest version might help?

Thanks for the help so far, since at least it works now which unblocks this PR :-)

@calixteman
Copy link
Contributor

It's a dep of express (what we use to run the server).
We use express 2.5.8 (2012) and mime 1.2.4 (2012 too).

@Snuffleupagus Snuffleupagus force-pushed the output-modules branch 2 times, most recently from 32116ac to 87562a6 Compare October 3, 2023 13:19
@mozilla mozilla deleted a comment from moz-tools-bot Oct 3, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Oct 3, 2023
@Snuffleupagus Snuffleupagus marked this pull request as ready for review October 3, 2023 14:51
@mozilla mozilla deleted a comment from moz-tools-bot Oct 4, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Oct 4, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Oct 4, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Oct 4, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Oct 4, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Oct 4, 2023
At this point in time all browsers, and also Node.js, support standard `import`/`export` statements and we can now finally consider outputting modern JavaScript modules in the builds.[1]

In order for this to work we can *only* use proper `import`/`export` statements throughout the main code-base, and (as expected) our Node.js support made this much more complicated since both the official builds and the GitHub Actions-based tests must keep working.[2]
One remaining issue is that the `pdf.scripting.js` file cannot be built as a JavaScript module, since doing so breaks PDF scripting.

Note that my initial goal was to try and split these changes into a couple of commits, however that unfortunately didn't really work since it turned out to be difficult for smaller patches to work correctly and pass (all) tests that way.[3]
This is a classic case of every change requiring a couple of other changes, with each of those changes requiring further changes in turn and the size/scope quickly increasing as a result.

One possible "issue" with these changes is that we'll now only output JavaScript modules in the builds, which could perhaps be a problem with older tools. However it unfortunately seems far too complicated/time-consuming for us to attempt to support both the old and modern module formats, hence the alternative would be to do "nothing" here and just keep our "old" builds.[4]

---
[1] The final blocker was module support in workers in Firefox, which was implemented in Firefox 114; please see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import#browser_compatibility

[2] It's probably possible to further improve/simplify especially the Node.js-specific code, but it does appear to work as-is.

[3] Having partially "broken" patches, that fail tests, as part of the commit history is *really not* a good idea in general.

[4] Outputting JavaScript modules was first requested almost five years ago, see issue 10317, and nowadays there *should* be much better support for JavaScript modules in various tools.
@mozilla mozilla deleted a comment from moz-tools-bot Oct 7, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Oct 7, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Oct 7, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Oct 7, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Oct 7, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Oct 7, 2023
@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/dfe42dc49ac2f66/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/dfe42dc49ac2f66/output.txt

Total script time: 1.39 mins

Published

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/7c6a24eda821954/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/44c40b0752466fc/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/44c40b0752466fc/output.txt

Total script time: 25.07 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 19
  different first/second rendering: 1

Image differences available at: http://54.241.84.105:8877/44c40b0752466fc/reftest-analyzer.html#web=eq.log

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/7c6a24eda821954/output.txt

Total script time: 35.80 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 26

Image differences available at: http://54.193.163.58:8877/7c6a24eda821954/reftest-analyzer.html#web=eq.log

Copy link
Contributor

@timvandermeij timvandermeij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me with the comment below addressed. Thank you for doing this!

Should we perhaps move the last commit to a new PR so that we can merge this now and merge the examples later after the 4.0 release?

src/display/node_utils.js Show resolved Hide resolved
@timvandermeij
Copy link
Contributor

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/fc1ae90259c5cd2/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/fc1ae90259c5cd2/output.txt

Total script time: 1.40 mins

Published

@timvandermeij
Copy link
Contributor

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/8525b7a7cf031d1/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/362a4930dbaa3d6/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/8525b7a7cf031d1/output.txt

Total script time: 5.45 mins

  • Integration Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Success

Full output at http://54.193.163.58:8877/362a4930dbaa3d6/output.txt

Total script time: 16.94 mins

  • Integration Tests: Passed

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

Successfully merging this pull request may close these issues.

publish es6 modules to npm
4 participants