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

Use a dynamic import instead of the <script> hack #1306

Merged
merged 1 commit into from Mar 27, 2023

Conversation

antocuni
Copy link
Contributor

This PR is based on #1258 and must be merged only after that. The only relevant commit is 57fed98.

As the title says, we want to use a dynamic import() to fetch and load pyodide, instead of dynamically adding a <script> tag to the page.

This greatly simplifies the logic for loading the interpreter and for handling user errors. In particular, until now we had to manually call handleUserError because afterInterpreterLoad was an async function called in response of a browser event.
Now, it's a normal async function called from another async function, so the error is automatically propagated up to main(), where it's correctly handled by the try/catch:

async main() {
try {
await this._realMain();
} catch (error) {
await this._handleUserErrorMaybe(error);
}
}

@WebReflection
Copy link
Contributor

WebReflection commented Mar 24, 2023

@antocuni I agree with the commented text and I am not a big fun of using modules to side-effect on the global context like we've always done (unless these are polyfills) ... although I think the commit "that matters" looks perfectly fine, so I'd be OK with that change as it surely simplifies the dance around importing that functionality 👍

P.S. I know Firefox is not there yet, but I hope we won't create too much documentation around this side-effect and switch to a proper ECMAScript module as soon as Firefox catches up with modules in workers 🤞

P.S.2 I can't somehow approve this MR for some reason 🤔 ... but maybe CI failing is to blame

@hoodmane
Copy link
Contributor

57fed98 looks good to me.

… logic around interpreter loading and handling of UserError
@antocuni antocuni force-pushed the antocuni/use-dynamic-import branch from 57fed98 to 2295b16 Compare March 27, 2023 15:45
@antocuni
Copy link
Contributor Author

@antocuni I agree with the commented text and I am not a big fun of using modules to side-effect on the global context like we've always done (unless these are polyfills) ... although I think the commit "that matters" looks perfectly fine, so I'd be OK with that change as it surely simplifies the dance around importing that functionality +1

P.S. I know Firefox is not there yet, but I hope we won't create too much documentation around this side-effect and switch to a proper ECMAScript module as soon as Firefox catches up with modules in workers

yes, even if I'm not a JS expert I can see that modules are clearly superiors. I'll be happy to switch to proper modules as soon as firefox catches up.


For the sake of future readers: I have rebased the branch on top of main, and what was known as commit 57fed98 is now 2295b16. I will merge it as soon as the tests are passing

@antocuni antocuni changed the title WIP: use a dynamic import instead of the <script> hack Use a dynamic import instead of the <script> hack Mar 27, 2023
@antocuni antocuni marked this pull request as ready for review March 27, 2023 15:50
Copy link
Contributor

@hoodmane hoodmane left a comment

Choose a reason for hiding this comment

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

So I guess the plan is that when we move to using a worker we can change the code here to:

if(inClassicWorker) {
   importScripts(interpreterURL);
} else {
   await import(interpreterURL);
}

@antocuni
Copy link
Contributor Author

So I guess the plan is that when we move to using a worker we can change the code here to:

if(inClassicWorker) {
   importScripts(interpreterURL);
} else {
   await import(interpreterURL);
}

Yes exactly

@antocuni antocuni merged commit 3ae4b3c into main Mar 27, 2023
4 checks passed
@antocuni antocuni deleted the antocuni/use-dynamic-import branch March 27, 2023 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants