Skip to content
This repository was archived by the owner on Mar 17, 2021. It is now read-only.

feat(index): add context to outputPath && publicPath #157

Closed
wants to merge 2 commits into from

Conversation

lsycxyj
Copy link

@lsycxyj lsycxyj commented Apr 30, 2017

I propose to add issuerContext to outputPath and publicPath to allow users change the resolved paths dynamically (eg. implementing relative path by myself).
BTW, the outputPath should be able to be changed by the users after using "useRelativePath" because it will emit files to wrong paths (eg. files will be placed to parent folder of output path when it starts with "..").

I propose to add issuerContext to outputPath and publicPath to allow users change the resolved paths dynamically (eg. implementing relative path by myself).
BTW,  the outputPath should can be changed by the users after using "useRelativePath" because it will emit files to wrong paths (eg. files will be placed to parent folder of output path when it starts with "").
@jsf-clabot
Copy link

jsf-clabot commented Apr 30, 2017

CLA assistant check
All committers have signed the CLA.

@lsycxyj
Copy link
Author

lsycxyj commented May 2, 2017

Knock, knock. Nobody's there?

@adriancmiranda
Copy link
Contributor

adriancmiranda commented May 2, 2017

@lsycxyj If approved this will been resolved at #150 😉

@lsycxyj
Copy link
Author

lsycxyj commented May 3, 2017

@adriancmiranda Thank you. I didn't mean that I only wanna fix the bug, but also wanna let the users handle the paths by themselves.

@michael-ciniawsky
Copy link
Member

@lsycxyj Sry we are shaping up a few things behind the scences in terms of CSS handling and this likely affects file/url-loader aswell, so they get stalled a bit atm, please be patient 😛

@lsycxyj
Copy link
Author

lsycxyj commented May 4, 2017

@michael-ciniawsky I'm looking forward to your good news. : )

@michael-ciniawsky
Copy link
Member

@lsycxyj Can you please provide a small example for this, just to clarify ? :)

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

Please add a test for this aswell

index.js Outdated
@@ -38,8 +38,8 @@ module.exports = function(content) {
var outputPath = "";

var filePath = this.resourcePath;
var issuerContext = this._module && this._module.issuer && this._module.issuer.context || context;
Copy link
Member

Choose a reason for hiding this comment

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

nitpick issuerContext => issuer || context (personally context)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, better clean this up.

Copy link
Author

Choose a reason for hiding this comment

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

@michael-ciniawsky Well... Actually this line of code is from the master branch. I just moved it outside the block. Do you mean I should choose issuer or context to pass to the publicPath function?

Copy link
Author

@lsycxyj lsycxyj Jun 3, 2017

Choose a reason for hiding this comment

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

@michael-ciniawsky My purpose of adding issuer's context to the publicPath function is to give the user a way to the resolved path trickily (eg. I can give a new relative path according to the issuer's context). So I prefer the issuer's context personally.

But I didn't fix the "useRelativePath" bug in this patch.

I notice that all the test cases are mocked cases, and there's no (good) way to find out problems caused by the webpack APIs (eg. this._module).
I'm not familiar with the webpack APIs. Maybe I need some help or advice to test it in a better way.

BTW, this._module has been deprecated by official docs. Maybe it should be changed in some other way?

@michael-ciniawsky michael-ciniawsky changed the title add issuerContext to outputPath and publicPath feat: add context to outputPath && publicPath Jun 2, 2017
Copy link
Member

@joshwiens joshwiens left a comment

Choose a reason for hiding this comment

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

Not really worth reviewing until a pull request has tests / negative tests covering the new functionality.

@joshwiens joshwiens modified the milestones: 0.11.2, 1.1.0 Jun 5, 2017
@joshwiens
Copy link
Member

@lsycxyj - I have set this to blocked ( #165 ) & moved this feature into 1.1.0, the next minor range after the pending 1.0.0 release.

@lsycxyj
Copy link
Author

lsycxyj commented Jun 7, 2017

@d3viant0ne @michael-ciniawsky Tests added.

@alexander-akait
Copy link
Member

@lsycxyj friendly ping

@lsycxyj
Copy link
Author

lsycxyj commented Feb 15, 2018

@evilebottnawi Is there anything I need to do?

@michael-ciniawsky michael-ciniawsky changed the title feat: add context to outputPath && publicPath feat(index): add context to outputPath && publicPath Aug 21, 2018
@alexander-akait
Copy link
Member

Done in #305

@lsycxyj
Copy link
Author

lsycxyj commented Dec 21, 2018

My idea was merged into the library but I'm not in the contributors list. 😢

@alexander-akait
Copy link
Member

@lsycxyj what do you mean?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants