-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Partially-complete support for "import" with JSObjectReference. #25198
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
Partially-complete support for "import" with JSObjectReference. #25198
Conversation
| // However since we're the one calling the import keyword, they would be resolved relative to | ||
| // this framework bundle URL. Fix this by providing an absolute URL. | ||
| if (typeof url === 'string' && url.startsWith('./')) { | ||
| url = document.baseURI + url.substr(2); |
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.
I did consider auto-prefixing regular base-relative URIs (e.g., myfile.js) with the base URI, but am reluctant to do that because we can't predict other possible syntaxes that browsers might add in the future.
However it is safe to rewrite URLs starting with ./ because they are unambiguous - we know for sure what that means, and there's no use case for ./ meaning relative to anything other than the base URI since at runtime the component itself doesn't have any location, and all our other URL-resolution behaviors are base relative too.
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.
I'm not sure what this bit means, can you give an explicit example?
| url = document.baseURI + url.substr(2); | ||
| } | ||
|
|
||
| return import(/* webpackIgnore: true */ url); |
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.
The webpackIgnore thing is needed because otherwise, when we build blazor.*.js, Webpack will replace the import call with some special gunk of its own that tries to find Webpack modules instead of doing an actual native dynamic import.
| */ | ||
| export function createJSObjectReference(jsObject: any): any { | ||
| if (jsObject instanceof Object) { | ||
| if (jsObject && typeof jsObject === 'object') { |
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.
This change is needed because an ES6 module isn't instanceof Object, but we do want such things to be usable as JSObjectReference values.
| export function createJSObjectReference(jsObject: any): any { | ||
| if (jsObject instanceof Object) { | ||
| if (jsObject && typeof jsObject === 'object') { | ||
| if (!jsObject.hasOwnProperty(JSObject.ID_KEY) || cachedJSObjectsById[nextJsObjectId] !== jsObject) { |
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.
Currently this line throws when jsObject is an ES6 module instance, because it doesn't have the hasOwnProperty function.
However, as per my comments here, I don't think we should be doing this stuff with JSObject.ID_KEY anyway, so this problem should go away on its own if that was fixed.
| "declaration": true, | ||
| "outDir": "dist" | ||
| "outDir": "dist", | ||
| "module": "ESNext", |
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.
This is needed because otherwise the TypeScript compiler interferes with import and tries to replace it with some old-school TypeScript-specific emulation of dynamic import. We don't want that - we just want the regular native import feature to be left intact.
MackinnonBuck
left a comment
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.
This looks good to me!
|
@MackinnonBuck Let me know how you'd prefer to proceed with this. I can't complete this directly on my own because it will only work after changes in #25028 as described above. Do you prefer to fold this into your PR #25028 and handle this all in one place? Or do you prefer me to do this as a separate PR afterwards? I'm fine with either! It would be good if you could check my implementation here works with your updated code otherwise we might need a further round of tweaks. |
|
@SteveSandersonMS I think it would work well to fold this into my PR so I could take care of all the changes at once 😃 |
I'm looking into what's needed to make JSObjectReference sufficient for our JS encapsulation needs.
It's not quite as trivial as I hoped - it's taken quite a while to figure out the magic combination of things needed. Also it's still not 100% working here, but the remaining changes are more to do with the JSObjectReference implementation so this is something to discuss with @MackinnonBuck.
Goal
Developers should be able to add an ES6 module, for example creating
myfile.jsinside theirwwwrootcontaining:... and then consume it from a Blazor component:
Of course, they can also store the
myModuleinstance and reuse it over time.