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

Refactor path resolve #116

Merged
merged 5 commits into from
Dec 24, 2015
Merged

Refactor path resolve #116

merged 5 commits into from
Dec 24, 2015

Conversation

TrySound
Copy link
Member

No description provided.

root: ".",
})
})

Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove this test(s)?
Looks pretty important to me.

@TrySound
Copy link
Member Author

@MoOx What is the case of local npm modules?

@MoOx
Copy link
Contributor

MoOx commented Dec 22, 2015

web_modules with webpack :)

@MoOx
Copy link
Contributor

MoOx commented Dec 22, 2015

Note that I think this also cover node_modules.

@TrySound
Copy link
Member Author

@MoOx package.json in node_modules or web_modules still works. But it doesn't work in local directories. Also I removed bower_components, caz we still do not support bower.json

@TrySound
Copy link
Member Author

@MoOx Okay I saved previous way to resolve paths.

@@ -18,7 +18,6 @@ var resolveMedia = require("./lib/resolve-media")
var moduleDirectories = [
"web_modules",
"node_modules",
"bower_components",
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be added in the CHANGELOG since it's a breaking changes (and we should add how to add bower support like it was).

@TrySound
Copy link
Member Author

@MoOx Done!

@TrySound
Copy link
Member Author

Is PR too big or you can accept it?

@@ -1,5 +1,9 @@
- Removed: async mode/option
([#107](https://github.com/postcss/postcss-import/pull/107))
- Removed: bower is not currently supported, use custom resolver
([#116](https://github.com/postcss/postcss-import/pull/116))
Copy link
Contributor

Choose a reason for hiding this comment

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

fyi, you can just add bower support by it to the path option.
So I would use "bower_components" not supported by default anymore, use "path" option to add it back.

Copy link
Member Author

Choose a reason for hiding this comment

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

bower support is also bower.json support, which is not simple with main field as array.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know that, but bower is more a downloader, so I guess lots of people just point to the right css file when they import it (eg: @import "jquery-slider-fx-2016/slider.css") so we don't really care about it. The idea is just to inform people that are currently using the feature we are breaking how to upgrade easily.

@MoOx
Copy link
Contributor

MoOx commented Dec 24, 2015

@TrySound Except my last comments about the CHANGELOG, this PR looks good. Great job. I just want to be sure that the developer experience for upgrading is easy :D

@TrySound
Copy link
Member Author

@MoOx Done

MoOx added a commit that referenced this pull request Dec 24, 2015
@MoOx MoOx merged commit 6eb3cd7 into dev Dec 24, 2015
@MoOx MoOx deleted the resolve-id branch December 24, 2015 08:31
@MoOx
Copy link
Contributor

MoOx commented Dec 24, 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.

3 participants