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

Fix: Async component loader wrong path for 'async!...' imports #50

Merged
merged 5 commits into from
May 25, 2017

Conversation

rkostrzewski
Copy link
Collaborator

Fixes #47

@@ -6,7 +6,14 @@ module.exports.pitch = function(remainingRequest) {
this.cacheable && this.cacheable();
var query = loaderUtils.getOptions(this) || {};
var routeName = typeof query.name === 'function' ? query.name(this.resourcePath) : null;
var name = routeName !== null ? routeName : ('name' in query ? query.name : (query.formatName || String)(this.resourcePath));
var name
if(routeName !== null) {
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: missing space after if keyword

@@ -51,6 +52,8 @@
"rules": {
"no-console": 1,
"no-empty": 0,
"semi": 2,
"keyword-spacing": 2,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@developit I took the liberty of adding those rules to eslint - please check if they're ok with you.
Semicolons are enforced (they were placed almost everywhere and not enforced) & spaces should be placed after keyword (no changes to files).

Copy link
Member

Choose a reason for hiding this comment

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

both good with me, thanks a bunch!

@@ -6,7 +6,14 @@ module.exports.pitch = function(remainingRequest) {
this.cacheable && this.cacheable();
var query = loaderUtils.getOptions(this) || {};
var routeName = typeof query.name === 'function' ? query.name(this.resourcePath) : null;
var name = routeName !== null ? routeName : ('name' in query ? query.name : (query.formatName || String)(this.resourcePath));
Copy link
Member

Choose a reason for hiding this comment

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

I knew someone would undo this lovely upsetting ternary

@developit
Copy link
Member

Are you able to shed light on what was messed up here? I'm happy to merge the change, just curious if it was a logic error or something I'm not seeing.

@rkostrzewski
Copy link
Collaborator Author

Previously this.resourcePath (which is an absolute URI) was passed to require.ensure third param which is chunk name - this in turn led to chunk names having whole absolute path as name ^^.

@developit developit merged commit 35fd84a into preactjs:master May 25, 2017
@rkostrzewski rkostrzewski deleted the fix/async-component-loader branch June 24, 2017 09:48
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.

3 participants