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

Change System.import() to import() #2163

Closed
guybedford opened this issue Mar 10, 2016 · 8 comments
Closed

Change System.import() to import() #2163

guybedford opened this issue Mar 10, 2016 · 8 comments

Comments

@guybedford
Copy link
Contributor

@guybedford guybedford commented Mar 10, 2016

I thought it would be worth mentioning this point for Webpack 2 consideration. The loader spec strictly specifies System.loader.import as the dynamic import entry point, instead of System.import as it used to be.

The other problem with using System.import is that strictly System.import shouldn't have any parent knowledge without a second argument. That is you'd really want System.import('x', parentName) to be the contextual require. Assuming System.import is already parent-scoped is going to not be true on the upgrade path at all. In SystemJS, we've defined a locally scoped __moduleName variable to allow for the above, but again it ends up being a custom solution.

In reality modules will have a contextual import function syntax available that will automatically scope the import to the current module parent so that this won't usually be necessary making for a nicer patter in ES modules. Perhaps it could be worth enquiring about that status of that syntax as well.

There might be the scope for a little more freedom to experiment here too. For example trying out one spec suggestion syntax like:

export function dynamicLoad() {
  return import.default('x') // default export (import.namespace being the normal namespace import)
}

which was discussed in whatwg/loader#36 (comment). If the spec does implement it, the upgrade path works. If the spec doesn't implement it, then it's just a breaking change to adjust, so it's a no different scenario than using System.import.

One could even then experiment with another possible syntax like:

export function async dynamicLoad() {
  return await import 'x';
}

And treating that import 'x' as the dynamic import. Would (import 'x').then(...) make sense with that grammar?

At some level it does come down to betting on something somewhat custom, and aiming to minimise the risk and inform users about the situation.

The approach SystemJS is taking here is not trying to polyfill System.loader due to potential conflicts when native implementations eventually land. Instead the idea is to stick with the "old" API of System.import and then double this up with a smooth upgrade path to a best practice non-conflict SystemJS.import, only switching into System.loader.import when we're sure there won't be a native conflict.

At least that's the best approach I've been able to come up with so far. If you've already considered this not to worry, but just to check the System.import decision should at least be made knowingly to avoid an upgrade path issue like SystemJS now has (although it would be great for SystemJS to be able to share the same convention actually, so I probably should have just shut up here!).

@TheLarkInn

This comment has been minimized.

Copy link
Member

@TheLarkInn TheLarkInn commented Sep 12, 2016

@guybedford I'm not sure how we missed this issue being submitted but we have this prioritized now. Thank you for putting this in.

@jhnns

This comment has been minimized.

Copy link
Member

@jhnns jhnns commented Sep 12, 2016

Thanks @guybedford for letting us know. This is the kind of collaboration I'm looking for. Most developers experience the upcoming standards through tools like webpack, SystemJS and babel. We should try to provide a friction-less upgrade path as much as possible 😀.

First of all, a promised-based solution for creating split points is very important. require.ensure (which was designed after an extension to the original CommonJS) has no way to report failures. Furthermore, it feels weird that you don't get back the actual modules, but still need to require it. That's why we need a new API for this.

But shipping an outdated spec knowingly feels also very wrong to me. This can be pretty confusing for developers. And since webpack@2 is still in beta, it's ok to change that until we remove the beta label.

I really like this proposal and it seems to be very up-to-date – which can be good and bad at the same time. Good, because it incorporates already some of the insights. Bad, because it has no broad support yet. If I understood it correctly it basically looks like this:

import("something").then(namespace => {
    namespace.someNamedExport;
    namespace.default;
});

Calling import() would be a syntax extension similiar to new.target and super() were a familiar notation is used for stuff you can't do with ordinary properties and functions. In this case, the context of the importing module is passed to import() under the hood without the developer needing to specify it. @guybedford Can you confirm my assumption?

Unfortunately, import() is no valid JavaScript for current parsers. That's why webpack will throw an error when encountering it. I'm not sure if we can extend acorn's parser to support import()?


This leaves us with two options:

  • Stick to System.import().
    • Pro: Same syntax as SystemJS (is that correct?). People can easily switch between tools.
    • Contra: Outdated.
    • Contra: Still has the notion of being something official. This can be very confusing for other developers who are not familiar with the spec.
  • Change the API of require.ensure(). Since webpack is the only popular implementation of require.ensure(), this is already conceived as webpack-specific syntax.
    • Pro: Can be easily integrated into webpack
    • Pro: Almost like the old implementation, but it returns a promise.
    • Pro: Is easily recognized as not an official thing
    • Contra: Looks uglier :)

Or does someone have a different solution?

@TheLarkInn

This comment has been minimized.

Copy link
Member

@TheLarkInn TheLarkInn commented Sep 12, 2016

cc @Rich-Harris also.

@guybedford

This comment has been minimized.

Copy link
Contributor Author

@guybedford guybedford commented Sep 13, 2016

As soon as the next TC39 meeting confirms the above import syntax which I believe is on the agenda, then it can be integrated into parsers and supported. It looks like this is the way things will be going now, and we can likely expect confirmation at the end of this month.

@jhnns

This comment has been minimized.

Copy link
Member

@jhnns jhnns commented Sep 13, 2016

That would be awesome!

@jhnns jhnns changed the title System.import v System.loader.import split point re webpack2 Change System.import() to import() Oct 19, 2016
@jhnns

This comment has been minimized.

Copy link
Member

@jhnns jhnns commented Oct 19, 2016

We talked about this at our last meeting.

Since we need a way to define split points in ES2015 modules but we also can't wait for the TC to confirm the spec before releasing webpack 2, we decided to use import() instead of System.import() to mark split points. I think, this makes sense since we know that the current syntax (System.import()) will be wrong anyway. So let's stick with the most likely (although we can't guarantee it).

@jhnns jhnns added P0: Critical and removed P1: Urgent labels Oct 19, 2016
@Jessidhia

This comment has been minimized.

Copy link
Member

@Jessidhia Jessidhia commented Oct 20, 2016

Looks related to #3098

@jhnns

This comment has been minimized.

Copy link
Member

@jhnns jhnns commented Oct 20, 2016

Yep, closing this one in favor of #3098

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
6 participants
You can’t perform that action at this time.