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

feat: add option to use output filepath for json #460

Merged
merged 1 commit into from Jul 25, 2018

Conversation

@morewry
Copy link
Contributor

@morewry morewry commented Jul 24, 2018

Adds an option to postcss-modular-css to automatically use the same output file path and file name for JSON with the configuration { json: true }. See #459.

@codecov
Copy link

@codecov codecov bot commented Jul 24, 2018

Codecov Report

Merging #460 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #460      +/-   ##
==========================================
+ Coverage   99.01%   99.02%   +<.01%     
==========================================
  Files          31       31              
  Lines         815      818       +3     
  Branches      129      130       +1     
==========================================
+ Hits          807      810       +3     
  Misses          8        8
Impacted Files Coverage Δ
packages/postcss/postcss.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d3257e...446666c. Read the comment docs.

Loading

Copy link
Owner

@tivac tivac left a comment

Thanks for jumping right onto this! It ended up being nice & small which is a pleasant surprise :)

Few small comments but nothing huge. This is really close!

Loading

@@ -24,14 +24,18 @@ module.exports = postcss.plugin("modular-css", (opts) =>
return processor.output();
})
.then((output) => {
var { json } = processor.options;

var { json, to } = processor.options;
Copy link
Owner

@tivac tivac Jul 24, 2018

Choose a reason for hiding this comment

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

I'm not sure this is the to we should be using. It's the same as the one from result.opts due to how the processor just greedily copies all the available options, but I think it'd be clearer to use the result.opts.to value instead.

That way if the processor ever goes crazy and starts changing to this plugin will still work w/ the correct value direct from postcss

Loading

Copy link
Contributor Author

@morewry morewry Jul 24, 2018

Choose a reason for hiding this comment

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

Okay, I'll update that.

Loading

result.messages.push({
type : "modular-css-exports",
exports : classes,
});

if(json) {
if(typeof json !== "string") {
json = to.replace(path.extname(to), ".json");
Copy link
Owner

@tivac tivac Jul 24, 2018

Choose a reason for hiding this comment

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

This indentation doesn't look like 4 spaces

Loading

Copy link
Owner

@tivac tivac Jul 24, 2018

Choose a reason for hiding this comment

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

Using .replace makes me a little nervous about files w/ weird names or paths, but is probably fine.

Loading

Copy link
Contributor Author

@morewry morewry Jul 24, 2018

Choose a reason for hiding this comment

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

Yeah, if it weren't for path.extname I wouldn't really trust it. If you'd prefer, can go to e.g. ${to}.json instead.

Loading

Copy link
Contributor Author

@morewry morewry Jul 24, 2018

Choose a reason for hiding this comment

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

Because, for example, I should probably ensure it's the very last one, rather than the first one, that gets replaced.

Loading

Copy link
Owner

@tivac tivac Jul 24, 2018

Choose a reason for hiding this comment

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

There's a (in theory) totally safe option that's really gross to read.

json = `${path.join(path.dirname(to), path.basename(to, path.extname(to))}.json`;

Loading

Copy link
Contributor Author

@morewry morewry Jul 24, 2018

Choose a reason for hiding this comment

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

I see what you mean about its readability. But I agree that it's preferable to, for example:

json = to.replace(new RegExp(`${path.extname(to)}$`), ".json");

It's actually clearer what it's doing and probably safer. Updated!

Loading

Adds an option to postcss-modular-css to automatically use the same
output file path and file name for JSON with the configuration
`{ json: true }`. See tivac#459.
@morewry morewry force-pushed the pr/jsonFilename branch from fedd669 to 446666c Jul 24, 2018
@morewry
Copy link
Contributor Author

@morewry morewry commented Jul 24, 2018

Updated to fix the indentation, use result.opts.to, and use path.join with path.dirname & path.basename to generate the filename.

Loading

tivac
tivac approved these changes Jul 24, 2018
Copy link
Owner

@tivac tivac left a comment

🎉🏆

Loading

@tivac tivac merged commit 037f933 into tivac:master Jul 25, 2018
3 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants