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

createScript / fetch / shouldFetch hooks #2058

Merged
merged 3 commits into from Jan 31, 2020
Merged

createScript / fetch / shouldFetch hooks #2058

merged 3 commits into from Jan 31, 2020

Conversation

guybedford
Copy link
Member

This implements three new hooks to open up the loader a little more. The docs contains the full explanation, but the gist is:

  1. Module types calls System.shouldFetch(), and then does a fetch based on that check. This allows eg setting up CSS modules for files not ending in .css, or ensuring all modules are fetched.
  2. Instead of calling global fetch, System.fetch is a global fetch alias, allowing the fetch function itself to be hooked. Effectively opening up to any custom transforms etc as an alternative to the transform hook.
  3. When doing a script load, createScript hooking allows hooking the script creation. This can be useful to inject custom authentication / integrity etc.

The hope is the above enables new types of userland plugins. The reality is no one will likely notice this PR even landed until one person in 6 months time decides they want the feature, and are grateful to find it. But hey I had a fun Sunday afternoon celebrating 10k stars.

@guybedford
Copy link
Member Author

This also fixes up the MIME checks in module types to properly enforce full MIME validation too.

function windowErrorListener(evt) {
if (evt.filename === url)
err = evt.error;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I've also refactored the global error listener.

});
}
else {
throw new Error('Unknown module type "' + contentType + '"');
Copy link
Member Author

Choose a reason for hiding this comment

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

With strict MIME checking, this now catches all unknown MIMEs, including the HTML error.

@guybedford
Copy link
Member Author

This resolves #1847 and #1863.

@@ -10,39 +10,40 @@ systemJSPrototype.register = function (deps, declare) {
systemRegister.call(this, deps, declare);
};

systemJSPrototype.createScript = function (url) {
const script = document.createElement('script');
script.charset = 'utf-8';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it save bytes to do Object.assign(script, {charset: 'utf-8', ...})

Copy link
Member Author

Choose a reason for hiding this comment

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

No Object.assign in IE11 :)

Copy link

Choose a reason for hiding this comment

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

Uncaught SyntaxError: Unexpected token export when import ES6 module

es6.js file
export var demo = "hello";

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @appsou, could you clarify what your comment means? Are you reporting a bug with a current version of systemjs? Or are you reporting a problem with this PR?

Note that systemjs does not support loading ES modules, as explained here and here. It primarily supports loading System.register modules.

src/features/script-load.js Show resolved Hide resolved
docs/hooks.md Show resolved Hide resolved
Copy link
Collaborator

@joeldenning joeldenning left a comment

Choose a reason for hiding this comment

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

lgtm

@frehner
Copy link
Contributor

frehner commented Nov 22, 2019

When doing a script load, createScript hooking allows hooking the script creation. This can be useful to inject custom authentication / integrity etc.

For what it's worth, I've wondered about adding integrity to the scripts that systemjs creates. Haven't necessarily needed it, but I wondered about how and if I could do it :)

@joeljeske
Copy link

Is anything blocking this PR?

@guybedford
Copy link
Member Author

@joeldenning what are your thoughts here?

@joeldenning
Copy link
Collaborator

joeldenning commented Jan 17, 2020

createScript makes a lot of sense, but with JSON, CSS, and HTML modules now having uncertain status, I don't know if it's a good idea to commit to a public hookable API (shouldFetch, fetch) for those kinds of modules. I think I'd lean towards only exposing createScript until there is more certainty around those module types, to avoid exposing an API we can't really stay behind as things change.

@guybedford
Copy link
Member Author

This doesn't just apply to module types though - it also permits an alternative to the transform loader model, and enables features such as custom fetch protocols (eg if someone wanted to implement the ability to fetch over RPC / with authorization etc).

To be honest those features are the more useful part of this PR than its effect on module types architecture, although for the module types architecture it does also handle #2086 as per #2086 (comment).

Copy link
Collaborator

@joeldenning joeldenning left a comment

Choose a reason for hiding this comment

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

Fair enough - in that case I think these are good changes.

@jdfrench44
Copy link

This feature would be helpful for me - I'm currently experiencing an issue where the browser isn't sending necessary cookies when requesting scripts, and it would be solved if I could set crossOrigin: 'use-credentials' in createScript hook.

@joeljeske
Copy link

Is there anything blocking the merge?

@joeldenning
Copy link
Collaborator

joeldenning commented Jan 31, 2020

No - just forgot to merge it. I'll merge master in and then merge this in.

@joeldenning
Copy link
Collaborator

I'll publish this as part of 6.2.0 once #2114 is reviewed and merged.

@joeldenning joeldenning merged commit f1e5ec5 into master Jan 31, 2020
@joeldenning joeldenning deleted the create-script branch January 31, 2020 20:55
@joeldenning
Copy link
Collaborator

Published in https://github.com/systemjs/systemjs/releases/tag/6.2.0

@jssoriao
Copy link

jssoriao commented Apr 8, 2020

do we have any examples of using this createScript hook to provide a bearer token? All I've seen is the default.

<script>
    System.constructor.prototype.createScript = function (url) {
        const script = document.createElement('script');
        script.charset = 'utf-8';
        script.async = true;
        script.crossOrigin = 'anonymous';
        script.src = url;
        return script;
      };
</script>

What should I do here to add the auth header?

@joeldenning
Copy link
Collaborator

Hi @jsoriao - we discussed this in the single-spa slack workspace at https://single-spa.slack.com/archives/C8R6U7MT7/p1586336163337900.

You cannot configure arbitrary headers to be sent with script tags, but you can tell the browser to automatically send the Authorization header by setting script.crossOrigin = 'credentials' and then having the server respond with WWW-Authorization header. Once the user is authenticated with that server, the browser will automatically send the correct header. Alternatively, you can use the shouldFetch and fetch hooks to manually set the authorization header's value.

@sscaff1
Copy link

sscaff1 commented Dec 17, 2021

How would I send a JWT token when fetching a script? That's not clear to me.
Instead of System.import, I would use System.fetch with the authorization header, but then how do I load the MFE?

  const applications = constructApplications({
    routes,
    loadApp({ name }) {
      // return System.import(name);
      // what do I do here instead of System.import?
    },
  });

@joeldenning
Copy link
Collaborator

@sscaff1 to tell SystemJS to use fetch() rather than <script>, create a shouldFetch hook that always returns true. Once you've done that, you can implement a fetch hook that adds a JWT token to the request.

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

8 participants