-
Notifications
You must be signed in to change notification settings - Fork 27
uses default export if require() returns an object #19
uses default export if require() returns an object #19
Conversation
@sokra I know you're very busy, but would you merge this or add me as a maintainer? I'll also take a stab at adding high order component support for es6 classes where mixins don't work. I'm greenjello on npm. |
require() almost always returns an object. I believe the export loader composition is the true fix. |
That's not true in babel 6 when using export syntax. // input
export default 1; // output
"use strict";
Object.defineProperty(exports, "__esModule", {
value: true
});
exports.default = 1; |
for non-Babel sources
|
@jsg2021 is correct, checking for an object primitive is not an adequate heuristic here @brigand, since this issue is specific to babel's cjs interop transform, it should be sufficient to check for the presence of the if (component.__esModule === true) component = component.default although this is going to be a common issue, i assume @sokra will prefer the exports loader solution suggested here.. either way, should definitely be documented |
Yeah, checking __esModule would be better, but it doesn't look like this will ever be merged so I won't bother updating the PR. I don't think exports loader is a good solution because you have to hardcode whether you're using es6 modules or not every place you use react-proxy (I don't have a .async.js extension or similar). This also means that when you disable babel's es6 module import and switch to webpack 2's es6 modules, you'll have to rewrite all the uses of react-proxy-loader (I think). |
It looks like there is a published fork here: Would this be the best to switch to for es6? |
Update: Webpack2 prevents the
|
@jsg2021 I ended up switching to https://github.com/luqin/react-router-loader Much easier, no config necessary, works well with es6 import. Here's a gist on jsx syntax with code splitting and dynamic routing https://gist.github.com/rosskevin/de55079e0d48f046fb518e45bb4f12b1 |
With webpack 2, the exports-loader chain no longer works :( So we will likely need a variant of this PR |
index.js
Outdated
' if(!component) {', | ||
' require.ensure([], function() {', | ||
' component = require(' + loaderUtils.stringifyRequest(this, moduleRequest) + ');', | ||
' if (typeof component === "object") component = component.default;', |
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.
lets update this to just if (component && component.default) component = component.default;
This will let component be any value, but if it is non-null and has a 'default' key it will take it over the original.
This being targeted towards React Components the 'component' value does not need to be as flexible as other loaders. We could even add a warning if the component isn't "a react renderable component"
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.
done
@brigand If you sign the cla, they will be able to merge your PR... also, if you see my comment above, it will be more universal. |
@jsg2021 updated and signed CLA. |
@d3viant0ne mind taking a look? |
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.
An example of when this happens:
It just checks to see if the export is an object, and uses
that.default
instead ofthat
. There's no compatibility break (previously would throw an error).