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

Set from option for stdin to process.cwd() + 'stdin.css' #115

Merged
merged 1 commit into from
Apr 1, 2017
Merged

Conversation

RyanZim
Copy link
Collaborator

@RyanZim RyanZim commented Apr 1, 2017

Should fix #114; will revert if this causes other issues.

@michael-ciniawsky @ai Please review

@coveralls
Copy link

coveralls commented Apr 1, 2017

Coverage Status

Coverage remained the same at 96.575% when pulling bec1a4b on stdin into 097f37a on master.

index.js Outdated
@@ -279,7 +279,7 @@ function css (css, file) {
if (file === 'stdin' && output) file = output

// TODO: Unit test this
if (file !== 'stdin') options.from = file
options.from = file === 'stdin' ? path.join(process.cwd(), 'stdin.css') : file
Copy link
Contributor

Choose a reason for hiding this comment

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

The dummy file stdin.css is a requirement or would process.cwd() just be fine also ? Sourcemaps won't work either way

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @ai 😛

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The issue at hand is that some plugins do path.dirname(options.from) to get the containing folder as the basis for resolving other files relative to the CSS. So yes, we need a dummy filename. Perhaps it should be stdin instead of stdin.css to avoid confusion (especially with SugarSS)?

@ai
Copy link
Member

ai commented Apr 1, 2017

LGTM

@RyanZim
Copy link
Collaborator Author

RyanZim commented Apr 1, 2017

@ai @michael-ciniawsky Thoughts on using stdin instead of stdin.css for the dummy filename?

@ai
Copy link
Member

ai commented Apr 1, 2017

Yeap, it will be better

Should fix #114; will revert if this causes other issues.
@RyanZim
Copy link
Collaborator Author

RyanZim commented Apr 1, 2017

Updated

@coveralls
Copy link

coveralls commented Apr 1, 2017

Coverage Status

Coverage remained the same at 96.575% when pulling 6081871 on stdin into 097f37a on master.

@RyanZim RyanZim merged commit 3daccc1 into master Apr 1, 2017
@RyanZim RyanZim deleted the stdin branch April 1, 2017 18:02
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.

browsers entry in package.json isn't respected
4 participants