Skip to content

[breaking change] Change parameter type of importModule from String to JSAny #55508

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

Closed
srujzs opened this issue Apr 18, 2024 · 9 comments
Closed
Assignees
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes web-js-interop Issues that impact all js interop

Comments

@srujzs
Copy link
Contributor

srujzs commented Apr 18, 2024

Change Intent

dart:js_interop's importModule should accept a JSAny instead of a String.

Original issue here: #55429

Justification

JavaScript's dynamic import function can accept more than just Strings. See https://tc39.es/proposal-dynamic-import/#sec-import-call-runtime-semantics-evaluation and https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/import#importing_modules_with_a_non-literal_specifier. One key usage is the ability to pass a TrustedScriptURL, allowing a sanitized specifier to be imported.

Impact

Minimal. There's only one usage in GitHub, and no usages in the SDK, google3, or Flutter.

Mitigation

The fix is to convert the String to a JSString using toJS e.g.

importModule('module') -> importModule('module'.toJS)

I don't believe mitigation is possible due to it being a parameter and not a return type. There's no way to "test" that a function takes in a specific type. One possibility that may work if it only needs to be mitigated for DDC and dart2js is to use dynamic to pass the value to importModule e.g.

importModule('module' as dynamic);

Since JSString is a Dart String when compiling to JS, this will avoid the breakage. This won't work for mitigating the issue for dart2wasm.

Change Timeline

This is a simple change and should go in Dart 3.5. No specific beta is planned yet.

Associated CLs

WIP CL here: https://dart-review.googlesource.com/c/sdk/+/363504

@srujzs srujzs added the breaking-change-request This tracks requests for feedback on breaking changes label Apr 18, 2024
@srujzs
Copy link
Contributor Author

srujzs commented Apr 18, 2024

@itsjustkevin FYI - I just sent out an email to announce@dartlang.org for both this and #55267.

@kevmoo
Copy link
Member

kevmoo commented Apr 18, 2024

Looks good to me!

@itsjustkevin
Copy link
Contributor

@Hixie @grouma @vsmenon please take a look.

@mraleph mraleph added the area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. label Apr 19, 2024
@sigmundch
Copy link
Member

lgtm as well

@sigmundch sigmundch added the web-js-interop Issues that impact all js interop label Apr 19, 2024
@grouma
Copy link
Member

grouma commented Apr 19, 2024

LGTM

@Hixie
Copy link
Contributor

Hixie commented Apr 19, 2024

no objection

@itsjustkevin
Copy link
Contributor

@vsmenon ping

@vsmenon
Copy link
Member

vsmenon commented Jun 5, 2024

lgtm

@itsjustkevin
Copy link
Contributor

This breaking change has been approved

@github-project-automation github-project-automation bot moved this from In review to Complete in Breaking Changes Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes web-js-interop Issues that impact all js interop
Projects
Status: Complete
Development

No branches or pull requests

8 participants