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

Brainstorm -- synchronously setting public path for webpack bundles. #1939

Closed
joeldenning opened this issue May 1, 2019 · 16 comments
Closed

Comments

@joeldenning
Copy link
Collaborator

joeldenning commented May 1, 2019

Problem

When consuming webpack bundles with SystemJS, it is necessary to dynamically set the webpack public path, so that webpack code splits know the base url to find other assets from. This cannot be set at build time, since the webpack bundle could be hosted on a variety of domains that aren't known at build time.

The System.resolve() api is great for giving you the url to use for your webpack public path. However, it is asynchronous when using system.js and import maps, which makes it impossible to set the public path before webpack starts processing its imports and code splits). This means that webpack will attempt to download code splits at the wrong url.

Repo that reproduces problem

See https://github.com/joeldenning/systemjs-webpack-public-path for a repo that clearly shows this problem. Of particular note is the set-public-path.js file.

Related

See "Use Case 1" in #1918 (comment). Also see webpack/webpack#8833 (comment) where it was decided that to start out webpack would continue downloading code splits via webpackJsonp instead of using SystemJS module.import().

Workaround

The url for the webpack public path can be synchronously known/calculated by monkey patching System.resolve and storing the resolved urls in a way you can access them synchronously:

const moduleMap = {};

window.getPublicPath = function(name) {
  const path = moduleMap[name];
  if (path) {
    return path.slice(0, path.lastIndexOf("/") + 1);
  } else {
    throw Error("Cannot find public path for " + name);
  }
};

const originalResolve = window.System.resolve;

window.System.resolve = function(name) {
  return originalResolve.apply(this, arguments).then(function(resolved) {
    moduleMap[name] = resolved;
    return resolved;
  });
};

Proposed solutions

Expose resolved urls via new System API
Create a resolveSync API that returns null if resolution hasn't happened or completed yet, but returns the string path if it has.

const fullUrl = System.resolveSync('main')
__webpack_public_path__ = fullUrl.slice(0, fullUrl.lastIndexOf('/') + 1)

Expose read-only import map synchronously

const fullUrl = System.getImportMap().imports.main
__webpack_public_path__ = fullUrl.slice(0, fullUrl.lastIndexOf('/') + 1)

Implement and use _context.meta.url
See _context.meta.url. This one would require the most work I think, since (it appears that) SystemJS does not currently provide _context in v3.1.6. Also, it would require a change to webpack that would make it possible for a webpack user to access _context.meta.url -- right now that's not really possible because webpack doesn't even know that _context exists.

System.register([], function(_export, _context) {
  const fullUrl = _context.meta.url
  __webpack_public_path__ = fullUrl.slice(0, fullUrl.lastIndexOf('/') + 1)
})
@frehner
Copy link
Contributor

frehner commented May 6, 2019

Both resolveSync and getImportMap seem nice from a user's perspective.

It's been awhile since I read through the import-map proposal, but I am under the impression that the import-map spec doesn't really have a clean way of figuring this out either, right? If that's true, it may be worth it to also open an issue there and start a conversation about it?

@guybedford
Copy link
Member

@joeldenning

Implement and use _context.meta.url

This seems to me a great route forward if it can work for you?

SystemJS definitely provides _context, and if it isn't that is a bug. This is implemented here - https://github.com/systemjs/systemjs/blob/master/src/system-core.js#L108.

Ideally you could just write:

__webpack_public_path__ = import.meta.url.slice(0, import.meta.url.lastIndexOf('/') + 1);

in your own code before it goes into webpack, and have Webpack turn that into a _context reference.

This would also provide better compatibility with the System format in Webpack itself.

Does that sound doable?

@joeldenning
Copy link
Collaborator Author

Implement and use _context.meta.url

This path is the hardest one -- right now webpack does not support import.meta and it looks like the expected behavior is somewhat unclear for it. See webpack/webpack#6719.

I think one of the tricky things to decide would be if import.meta would be _context only for the webpack entry module or in all modules in the bundle. I think that only the entry module would make sense, but we'd have to see if the webpack team agrees with that.

Going down this path would also mean that people having webpack compile to AMD/UMD would not be able to get the resolved url synchronously. They would have to switch to System format.

@guybedford
Copy link
Member

In AMD/UMD it's available via require.uri and we could make that available in the AMD format again.

@guybedford
Copy link
Member

Sorry it's module.uri in AMD.

@joeldenning
Copy link
Collaborator Author

Ah I didn't know about module.uri.

If this is the best option, I can start looking into what it would take to PR webpack to support import.meta. The process would likely take a fair amount of time, to see if this kind of implementation fits into what the webpack team envisions for import.meta.url, and then implementing a PR that is able to parse for import.meta.url and then swap it out with the correct value for AMD/UMD/System.

My guess is it would take at least a few months before it were in a webpack release, but I can try it 😄

Could you comment on the other proposed solutions, and what you see as pros/cons of them?

@joeldenning
Copy link
Collaborator Author

Main con is just adding another System api that is niche?

@guybedford
Copy link
Member

I agree import.meta contextual usage typically should be optimized out by build tools - eg import.meta.url would typically correspond to asset emission relative to the input module.

So having it correspond to the output doesn't necessarily make sense you're right.

And then tracking that into Webpack isn't a straightforward feature, yes.

It seems annoying though as the context is all there... just not accessible! Could you just make the _context argument an actual __system_context__ argument or something?

As for the other proposals, I don't want to expose the import map as that would need to be a clone to avoid mutations, which seems expensive, and also inspection of the import maps is not currently a feature of import maps, so exposing it in SystemJS shouldn't be done lightly.

For a syncResolve function, we could have like a core syncResolve which the asyncResolve wraps. I guess something around that could be worked out. But I'm just so weary of what happens when they get out of sync which seems like it would happen so easily? Especially when we introduce dynamic import map loading. Having a function that bails with an error seems non-ideal.

Would top-level await help your use case? Because the chunk could be deferred on async code? Perhaps implementing top-level await in webpack is an option too!? (System format supports it via async declaration function).

@joeldenning
Copy link
Collaborator Author

joeldenning commented May 12, 2019

Would top-level await help your use case?

hmm maybe, although I don't think so. With the following example, I think that (within webpack) a.js would execute before the public path is set:

await System.resolve('my-entry-module')
  .then(url => __webpack_public__path = url.slice(0, url.lastIndexOf('/') + 1)
import './a'

Could you just make the _context argument an actual __system_context__ argument or something?

That's is a pretty good idea -- that PR to webpack would be a lot easier to implement and probably a lot easier for the webpack team to accept. I like that as the best option. I'll try out a webpack PR for it.

@joeldenning
Copy link
Collaborator Author

I just created a PR in webpack for this ^

It turned out to be a one-liner that I hope is easy for the maintainers to accept.

@joeldenning
Copy link
Collaborator Author

@guybedford after more thought, I think we should revisit this problem again. I think the webpack PR is a good one to get in there.

However, for people on webpack@<4.30 or for those who simply do not know about output.libraryTarget system or choose not to use it, the original problem described in this issue remains.

My other two proposed solutions are System.resolveSync('main') or System.getImportMap(). Do either of seem good to you, or do you have another alternative?

@guybedford
Copy link
Member

@joeldenning I believe the intention will be to expose individual import maps as the .importmap JSON property of the <script type="importmap"> scripts in the page. Perhaps SystemJS can assign such a property as it parses each import map? That would also get around the issue of having to clone the import map since these would be the individual maps before composition.

@joeldenning
Copy link
Collaborator Author

Yeah an .importmap property would be a good step. To at least avoid duplicate network requests if other code wants to calculate the final import map.

@joeldenning
Copy link
Collaborator Author

joeldenning commented Jul 24, 2019

Here's my workaround for now. Pretty nasty, but works.

<!-- in your index.html file, before you do any System.import() calls -->
<script>
  (function() {
    var originalResolve = System.resolve
    var moduleMap = {}
    System.resolve = function(name) {
      return originalResolve.apply(this, arguments).then(resolved => {
        moduleMap[name] = resolved;
        return resolved;
      });
    }
    window.getModuleUrl = function(name) {
      if (moduleMap[name]) {
        return moduleMap[name];
      } else {
        throw Error(`Could not find url for module '${name}'`)
      }
    }
  }()
</script>
// At the very very top of your webpack entry file
import './set-public-path';
// The set-public-path.js file
const fullUrl = window.getModuleUrl('nameOfModuleInImporMap')
__webpack_public_path__ = fullUrl.slice(0, fullUrl.lastIndexOf('/') + 1)

@guybedford
Copy link
Member

@joeldenning I really like us moving this approach over to #1985 since that also aligns with the import maps spec and how dynamic import map injection might work too.

Would you be happy to close this to track the feature there?

@joeldenning
Copy link
Collaborator Author

joeldenning commented Aug 23, 2019

Yeah let's close it and use #1985 to track. For any future readers of this thread, the proposed change will make it so you can call System.resolve synchronously to find your webpack public path

const systemjsUrl = System.resolve('my-module-name')
__webpack_public_path__ = systemjsUrl.slice(0, systemjsUrl.lastIndexOf('/') + 1)

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

No branches or pull requests

3 participants