-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
ESM ScriptType #5963
ESM ScriptType #5963
Changes from 9 commits
60cda4f
b60ca97
25dcfa0
2ee13cb
f9013b7
f5bdda0
91aae0b
2782805
de9e27e
d0623e4
3931ddd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,8 @@ | ||
import { platform } from '../../core/platform.js'; | ||
import { script } from '../script.js'; | ||
import { ScriptType } from '../script/script-type.js'; | ||
import { ScriptTypes } from '../script/script-types.js'; | ||
import { registerScript } from '../script/script.js'; | ||
import { ResourceLoader } from './loader.js'; | ||
|
||
/** @typedef {import('./handler.js').ResourceHandler} ResourceHandler */ | ||
|
@@ -53,7 +56,7 @@ class ScriptHandler { | |
const self = this; | ||
script.app = this._app; | ||
|
||
this._loadScript(url.load, (err, url, extra) => { | ||
const onScriptLoad = (url.load, (err, url, extra) => { | ||
if (!err) { | ||
if (script.legacy) { | ||
let Type = null; | ||
|
@@ -88,6 +91,28 @@ class ScriptHandler { | |
callback(err); | ||
} | ||
}); | ||
|
||
// check if we're loading a module or a classic script | ||
const [basePath, search] = url.load.split('?'); | ||
const isEsmScript = basePath.endsWith('.esm.js'); | ||
|
||
if (isEsmScript) { | ||
|
||
// The browser will hold its own cache of the script, so we need to bust it | ||
let path = url.load; | ||
if (path.startsWith(this._app.assets.prefix)) { | ||
path = path.replace(this._app.assets.prefix, ''); | ||
} | ||
|
||
const hash = this._app.assets.getByUrl(path).file.hash; | ||
const searchParams = new URLSearchParams(search); | ||
searchParams.set('hash', hash); | ||
const urlWithHash = `${basePath}?${searchParams.toString()}`; | ||
|
||
this._loadModule(urlWithHash, onScriptLoad); | ||
} else { | ||
this._loadScript(url.load, onScriptLoad); | ||
} | ||
} | ||
|
||
open(url, data) { | ||
|
@@ -120,6 +145,38 @@ class ScriptHandler { | |
|
||
head.appendChild(element); | ||
} | ||
|
||
_loadModule(url, callback) { | ||
|
||
// if we're in the browser, we need to use the full URL | ||
const baseUrl = platform.browser ? window.location.origin : import.meta.url; | ||
const importUrl = new URL(url, baseUrl); | ||
|
||
// @ts-ignore | ||
import(importUrl.toString()).then((module) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this uses import, which based on https://caniuse.com/?search=import is supported by Chrome from 2017, Safari 2018, Firefox 2019 - I wonder how we could handle the compatibility issues here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens on browsers using old script style .. are they going to get exception when this code gets parsed? Or is there some babel polyfill for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There has to be an assumption that if a user chooses to use ES modules, then there is a neccessary baseline level of support required. ES6 is around 97.2% which is only 0.7% less than our legacy build. If someone uses the old script mechanism, the ESM path won't get triggered so it will just work as is. |
||
|
||
for (const key in module) { | ||
const scriptClass = module[key]; | ||
const extendsScriptType = scriptClass.prototype instanceof ScriptType; | ||
|
||
if (extendsScriptType) { | ||
|
||
if (script.attributesDefinition) { | ||
for (const key in script.attributesDefinition) { | ||
scriptClass.attributes.add(key, script.attributesDefinition[key]); | ||
} | ||
} | ||
|
||
registerScript(scriptClass, scriptClass.name); | ||
} | ||
} | ||
|
||
callback(null, url, null); | ||
|
||
}).catch((err) => { | ||
callback(err); | ||
}); | ||
} | ||
} | ||
|
||
export { ScriptHandler }; |
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.
Can an es5 version of the engine load module scripts? If so then
import.meta.url
would fail, right?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.
(Even though that'll only ever be used in node).
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 target is marginally higher than ES5 now, but babel should transpile this according to the preset.env. See here
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.
That sounds good, just double check it does and we're not missing some settings please.