Improve documentation of implementing instantiate(), notice about race condition
#2265
Conversation
File size impactextras (0)No impact on files in extras group. browser (0)No impact on files in browser group. node (0)No impact on files in node group. |
docs/hooks.md
Outdated
| By default, a SystemJS module is not marked by an "identifier" (to match ES Module), therefore SystemJS have to use a race condition prone way to register the module. So you must make sure to maintain the execution order of the code. | ||
|
|
||
| The [module loading process](https://github.com/systemjs/systemjs/blob/master/src/system-core.js#L65-L77) is like: |
There was a problem hiding this comment.
| By default, a SystemJS module is not marked by an "identifier" (to match ES Module), therefore SystemJS have to use a race condition prone way to register the module. So you must make sure to maintain the execution order of the code. | |
| The [module loading process](https://github.com/systemjs/systemjs/blob/master/src/system-core.js#L65-L77) is like: | |
| By default, SystemJS modules are not registered by name to match ES Modules. Instead, registration of modules is done by matching a `System.register` call to its caller URL using a synchronous callback. | |
| See below for an outline of the loading lifecycle provided by this default `instantiate` implementation: |
There was a problem hiding this comment.
a synchronous callback.
It doesn't have to be synchronous, what really important is the order, in my implementation for WebExtension, the callback is async but I maintain the order so things won't break.
|
|
||
| The [default implementation](https://github.com/systemjs/systemjs/blob/master/src/features/script-load.js) is based on the creation of `<script>` tag and the `onload` event is emitted just after the System.register call. | ||
|
|
||
| If in your environment it is not possible to maintain the execution order (for example, involves the interaction of multiple event loops), you can make the call to the instantiate in a sequent way (Notice: this will slow down the module loading speed if they need to loaded by the network). Here is another [example that implementing System.instantiate](https://github.com/Jack-Works/webextension-systemjs/blob/master/src/content-script.ts#L10-L26) for a speical environment that cannot use `<script>` tag. |
There was a problem hiding this comment.
Let's rather link to the src/features/fetch-loader.js implementation here as an example.
There was a problem hiding this comment.
There was a problem hiding this comment.
oh it's https://github.com/systemjs/systemjs/blob/master/src/features/fetch-load.js my branch seems like outdated
There was a problem hiding this comment.
fetch-load is calling getRegister immediately after eval and not showing how to handle the race condition (cause it doesn't have race condition).
My webextension-systemjs involved this topic (cause I'm not using eval but browser.tabs.executeScript which is async and not reliably to get a callback of execution done).
There was a problem hiding this comment.
If you're not calling getRegister immediately after eval then you will have race conditions.
There was a problem hiding this comment.
It's impossible to know if the script executed synchronously in some environments. I do have race conditions in my early implementation of webextension-systemjs but I successfully workaround on it.
https://github.com/Jack-Works/webextension-systemjs/blob/master/src/content-script.ts#L10-L26, it chains the current promise after the last execution (to make it serial) therefore to maintain the correct order (and still keep asynchronous).
There was a problem hiding this comment.
Can you really not just call eval manually? Promise resolution runs the task queue, which means you always have a very small window where a race can still happen even if it hasn't happened in testing, it could be 1 in 100 or 1 in 10000 but the race is still there.
There was a problem hiding this comment.
I used it in our products directly, and the current version works well. eval is banned in our environment by CSP policy (banning eval is enabled by default in WebExtension).
There was a problem hiding this comment.
What's the reason you can't attach a sync callback in the page with eg a page registration queue handler that messages back both the registration and url together?
There was a problem hiding this comment.
Sorry, I didn't get your idea 馃憖 can you write some pseudo-code to explain?
Co-authored-by: Guy Bedford <guybedford@gmail.com>
sync with master
|
hmm, hello? |
|
Please use a decent tone here. "Hmm hello" is not a way to speak to a maintainer. Contributors are always welcome to the project, but it requires being constructive. Non constructive comments will be blocked and issues locked. |
|
Oh I'm sorry if I used offensive tone or words. I'm not native speaker so I can't feel that emotion clearly |
|
It's important to just be respectful of a maintainers time. So far this PR has cost me many minutes of work, and there has been nothing in return. The PR is still not of the quality to merge. I don't think further feedback is going to help. Closing, but appreciated for the thought of contributing. |
|
Fine, feel free to pick any quality changes (if there is any) to improve the documents if you'd like to. |
continue of #2238
馃憖