-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Conversation
b856022
to
469789e
Compare
Given the errors when loading the previews above, my guess would be that the server running on the bots doesn't understand the Line 26 in be53c7d
@calixteman Can you please check if this can be fixed on the bots? |
/botio-linux preview |
acf25b4
to
9c8232b
Compare
So it works but I had to modify |
What version of the Thanks for the help so far, since at least it works now which unblocks this PR :-) |
It's a dep of |
32116ac
to
87562a6
Compare
87562a6
to
269a5c3
Compare
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.
378f833
to
0b5f70d
Compare
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/dfe42dc49ac2f66/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/dfe42dc49ac2f66/output.txt Total script time: 1.39 mins Published |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.193.163.58:8877/7c6a24eda821954/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/44c40b0752466fc/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/44c40b0752466fc/output.txt Total script time: 25.07 mins
Image differences available at: http://54.241.84.105:8877/44c40b0752466fc/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/7c6a24eda821954/output.txt Total script time: 35.80 mins
Image differences available at: http://54.193.163.58:8877/7c6a24eda821954/reftest-analyzer.html#web=eq.log |
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.
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?
0b5f70d
to
927e50f
Compare
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://54.241.84.105:8877/fc1ae90259c5cd2/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/fc1ae90259c5cd2/output.txt Total script time: 1.40 mins Published |
/botio integrationtest |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @timvandermeij received. Current queue size: 0 Live output at: http://54.241.84.105:8877/8525b7a7cf031d1/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @timvandermeij received. Current queue size: 0 Live output at: http://54.193.163.58:8877/362a4930dbaa3d6/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/8525b7a7cf031d1/output.txt Total script time: 5.45 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.193.163.58:8877/362a4930dbaa3d6/output.txt Total script time: 16.94 mins
|
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.