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: use correct options object & set fallback context #20

Merged
merged 2 commits into from
Apr 11, 2018

Conversation

herschel666
Copy link
Contributor

Hey, I have two fixes here. :-)

@olegstepura
Copy link
Owner

Could you please describe what it fixes

@herschel666
Copy link
Contributor Author

herschel666 commented Mar 9, 2018

When using the loader with webpack@4.1.1 on node@8.9.1 I'll get the following error:

TypeError: Cannot read property 'context' of undefined

My guess was, that this is related to treating the options as an instance variable, while it's actually a local variable. After fixing that, I got the following error:

TypeError: Path must be a string. Received undefined

This is because options.context, which is passed into path.relative(), is undefined. My take was to set the falback for context to process.cwd().

An alternative approach would be to fall back to an empty string:

- this.emitFile(path.relative(options.context, content.outputFilePath), content.contents || [''], map);
+ this.emitFile(path.relative(options.context || '', content.outputFilePath), content.contents || [''], map);

What do you think?

@olegstepura
Copy link
Owner

Did you try to use queryOptions to define options.context to fix you issue?

@herschel666
Copy link
Contributor Author

No, since it's not a required option parameter.

@unstubbable
Copy link

I'm also interested in this fix. In my opinion the proposed solution makes sense. Setting the context via query options does not work, unless at least the second part of this PR is accepted:

- this.emitFile(path.relative(this.options.context, content.outputFilePath), content.contents || [''], map);
+ this.emitFile(path.relative(options.context, content.outputFilePath), content.contents || [''], map);

@unstubbable
Copy link

unstubbable commented Apr 11, 2018

Looking at other PRs (like this one or this one), I think this is the correct fix:

- this.emitFile(path.relative(this.options.context, content.outputFilePath), content.contents || [''], map);
+ var context = options.context || this.context || this.rootContext;
+ this.emitFile(path.relative(context, content.outputFilePath), content.contents || [''], map);

And also initialising options with an empty object (var options = {};).

Copy link

@unstubbable unstubbable left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@herschel666
Copy link
Contributor Author

@KingHenne man, you're fast! :-D

@olegstepura olegstepura merged commit d5fc994 into olegstepura:master Apr 11, 2018
@olegstepura
Copy link
Owner

olegstepura commented Apr 11, 2018

Ok, merged and released 0.0.12

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