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

Support loader prefixes when resolving paths #28

Closed
wants to merge 2 commits into from

Conversation

kpdecker
Copy link
Contributor

@kpdecker kpdecker commented Jan 8, 2015

No description provided.

kpdecker added a commit to walmartlabs/circus-stylus that referenced this pull request Jan 8, 2015
Should be reverted once webpack-contrib/stylus-loader#28 is released.

Fixes #3
@PatrickJS
Copy link

+1

@mzgoddard
Copy link
Collaborator

I think we can do this. @kpdecker the resolver file seems out of date with the latest stylus 0.49.3 which we have as our peer dependency. If so can you update that file, put the stylus version at the top of the file with your comment and put in comments like // stylus-loader: BEGIN DIFFERENCE WITH SYTLUS and a similar one to end to signify the blocks of that file that are different.

Thanks!

@kpdecker
Copy link
Contributor Author

@mzgoddard this deviates from the stylus file fairly dramatically, some from me refactoring, some from functional changes:
https://gist.github.com/kpdecker/7d9fbe212a11ec2efd9f

I think that trying to shoehorn into the pattern that they are following there will hurt the readability of the code vs. just calling it a divergent fork of this file.

@mzgoddard
Copy link
Collaborator

@kpdecker I think you have a really great point. This seems difficult to keep up to date with mainline stylus since its essentially a forked file. I've been thinking about how we can make this maintainable. I don't think I've thought of a great answer that lets us take immediate use of your work with little change. Any thoughts you may have on this would be great. If we can't think of anything I think I'll go ahead and merge after this week since this is definitely useful work.

Thanks for everything so far.

@kpdecker
Copy link
Contributor Author

Not sure that I'm aware of anything and I'm without laptop on vacation, so
I'm not sure I can provide better feedback right now.

On Tue Feb 17 2015 at 2:31:22 PM mzgoddard notifications@github.com wrote:

@kpdecker https://github.com/kpdecker I think you have a really great
point. This seems difficult to keep up to date with mainline stylus since
its essentially a forked file. I've been thinking about how we can make
this maintainable. I don't think I've thought of a great answer that lets
us take immediate use of your work with little change. Any thoughts you may
have on this would be great. If we can't think of anything I think I'll go
ahead and merge after this week since this is definitely useful work.

Thanks for everything so far.


Reply to this email directly or view it on GitHub
#28 (comment).

@mzgoddard
Copy link
Collaborator

Merged as ab62a5f. Sorry for the delay.

@mzgoddard mzgoddard closed this Mar 2, 2015
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.

None yet

3 participants