Add sourcemap, nesting, and css-modules CLI flags #68

rrcobb · 2022-01-20T19:35:28Z

Address parts of #52, but not --targets or transforming multiple files.

makes --minify a boolean option, instead of a subcommand
instead of --input-file option as part of the minify subcommand, input file is now a required positional arg
moves --output-file from subcommand to the top level
adds --nesting boolean option, to enable nesting
adds --css-modules and --css-modules-output-file options
adds --sourcemap option
adds integration tests in rust for the CLI in tests/cli_integration_tests.rs

CSS Modules and sourcemap options make what seem to me like decent default choices for where to send their output.

Sourcemap requires an output file, since it doesn't make sense to have a sourcemap without a file...
if there's an output file out.css, then the map is written to out.css.map
css modules allows specifying a path for the json output, or writes to out.json if there's an output file named out.css

I'd be glad for any feedback, and happy to carve it into smaller chunks, in case this PR is too large.

height · 2022-01-20T19:35:30Z

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

rrcobb · 2022-01-20T19:52:06Z

src/main.rs

+#[derive(Serialize)]
+#[serde(rename_all = "camelCase")]
+struct SourceMapJson<'a> {
+  version: u8,
+  mappings: String,
+  sources: &'a Vec<String>,
+  sources_content: &'a Vec<String>,
+  names: &'a Vec<String>
 }


This and the code for actually writing the sourcemap json are pulled almost exactly from node/src/lib.rs. I didn't pull out a common module, since I'm not totally sure how the project likes to organize the code, but there's a common bit of code to pull out, potentially... maybe into parcel_sourcemap itself, but I'm not sure.

Yeah Think putting it in parcel_sourcemap would be good eventually. Doesn't have to happen for this PR though.

rrcobb · 2022-01-20T19:56:17Z

src/main.rs

+    let filename = path::Path::new(&cli_args.input_file)
+        .file_name()
+        .unwrap()
+        .to_str()
+        .unwrap()
+        .into();


When running the CLI twice on the same file, but with different locations, the input_file arg is different, even if the css source code is the same. That meant there'd be different hashes in for the css module output! This normalizes the filename for the stylesheet, which means the hash is more consistent.

I'm still pretty new to the codebase, so it could be that there's some downstream effects of this that I'm not expecting.

If there's a nicer way to accomplish this in rust, I don't love these unwraps...

I think we should probably avoid this. For example, if you had two different files in your project each named index.css or something but in different folders, the class names could clash. Using the full absolute path (or a relative one from some project root) avoids this.

Oh, that's a good point, that does seem like it will definitely happen, especially once there's multiple file support!

I guess a pertinent question is whether invoking from different locations, the output should be deterministic. I still think yes (though, it's clear that file_name doesn't do the job here). Is there something else to hash that makes sense? The source file contents?

rrcobb · 2022-01-20T19:57:28Z

src/main.rs

+    let map = if let Some(ref mut source_map) = res.source_map {
+        source_map.set_source_content(0, &res.code).map_err(|_| io::Error::new(io::ErrorKind::Other, "Error setting sourcemap"))?;
+        let mut vlq_output: Vec<u8> = Vec::new();
+        source_map.write_vlq(&mut vlq_output).map_err(|_| io::Error::new(io::ErrorKind::Other, "Error writing sourcemap vlq"))?;
+
+        let sm = SourceMapJson {
+            version: 3,
+            mappings: unsafe { String::from_utf8_unchecked(vlq_output) },
+            sources: source_map.get_sources(),
+            sources_content: source_map.get_sources_content(),
+            names: source_map.get_names()
+        };
+
+        serde_json::to_vec(&sm).ok()


almost line for line from node/src/lib.rs, but with different map_err, since we don't have the CompilerError enum here

rrcobb · 2022-01-20T20:01:08Z

src/main.rs

+        if cli_args.css_modules {
+            let css_modules_json = serde_json::to_string(&res.exports)?;
+            println!("{}", css_modules_json);


printing css modules to stdout feels a little weird, but I guess if you add the --css-modules flag, you're expecting css modules, so better to get it than not?

Maybe better to require the --output-file flag in order to turn on --css-modules. Thoughts?

Yeah I think requiring output file for css modules makes sense. Same with source maps, unless we inline the whole thing. I guess the main usecase for stdout is to pipe it somewhere, so probably most useful if that only includes the css.

I'll make this switch! Drop the complexity / printing here, and make an output file required for using the css modules flag.

rrcobb · 2022-01-20T20:02:15Z

src/main.rs

+fn infer_css_modules_filename(output_file: Option<String>) -> Result<String, std::io::Error> {
+    if let Some(file) = output_file {
+        let path = path::Path::new(&file);
+        if path.extension() == Some(ffi::OsStr::new("json")) {
+            Err(io::Error::new(io::ErrorKind::Other, "Cannot infer a css modules json filename, since the output file extension is '.json'"))
+        } else {
+            // unwrap: the filename option is a String from clap, so is valid utf-8
+            Ok(path.with_extension("json").to_str().unwrap().into())
+        }
+    } else {
+        Ok("styles.json".into())
+    }


on rereading this, I don't actually think we ever infer the filename if we don't have an output file specified; this should take a string and shouldn't have the outer condition

rrcobb · 2022-01-20T20:02:53Z

tests/cli_integration_tests.rs

+    file.write_str(
+        r#"
+      .foo {
+        border: none;
+      }
+    "#,


Borrowed test values from src/lib.rs

devongovett · 2022-01-21T05:36:50Z

Skimmed it and it looks great. Thanks for writing tests as well! Will look in more detail tomorrow.

joelmoss · 2022-01-21T13:28:11Z

Does this support sending the output of CSS Modules JSON and CSS to stdout?

devongovett · 2022-01-21T14:56:36Z

How would that work and what is the usecase for that? How would you separate them? I assume the usecase for stdout is piping to another program, but if we mix CSS and JSON that will be harder...

joelmoss · 2022-01-21T15:06:05Z

Yeah so that is the tricky part.

My use case is to call the CLI and pipe it to my app. I'm building a Rails middleware that will intercept requests for CSS files and return the CSS file - basically a proxy. CSS files will be built on demand using the parcel-css CLI.

But (at least in development) it is more efficient and faster to return the contents directly from the CLI, as opposed to the CLI writing to a file(s) and my app opening and rendering file contents of the file.

Instead, my app calls the CLI and returns exactly what the CLI returned, and/or be able to work with the returned data.

So when this is just CSS, all is well. But when we have JSON and CSS - like we do with CSS modules - then it's tricker. I would still prefer not to read/write files.

So maybe in this case, we return everything as json:

{
  "css": ".header_abc123 { color: red; }",
  "modules": {
    "header": "header_abc123"
  }
}

This makes it trivial for the caller to work with the returned data.

Hope that helps shed some light on what I'm after. thx

devongovett · 2022-01-22T17:09:15Z

Going to merge this and do a few fixups separately. Thanks again so much for the contribution!

rrcobb · 2022-01-22T18:41:50Z

How would that work and what is the usecase for that? How would you separate them? I assume the usecase for stdout is piping to another program, but if we mix CSS and JSON that will be harder...

Yeah, I think piping the css modules json file to stdout is wrong; it's probably better to leave it out unless the output file is specified.

rrcobb · 2022-01-22T18:45:13Z

Oh, @devongovett you already shipped the fixups! Nice!

lmk if there's other pieces I can chip in on, I'm starting to get a sense for the codebase, and I like it!

devongovett · 2022-01-22T18:49:56Z

@rrcobb Thanks! A few things we could add if you're up for any of them:

Support for compiling multiple files at once
Support for --targets
Nicer error messages. Right now we just crash due to the unwrap but would be nice to have some pretty code frames e.g. https://github.com/zkat/miette
Maybe a --json option to output CSS + source map + css modules as a JSON object to stdout? I think that might satisfy what @joelmoss is looking for.

I'm also currently working on a very basic --bundle option that will inline @import rules.

joelmoss · 2022-01-22T19:03:20Z

Maybe a --json option to output CSS + source map + css modules as a JSON object to stdout? I think that might satisfy what @joelmoss is looking for.

That would awesome 👏

joelmoss · 2022-02-01T18:24:02Z

Just thinking out loud here, but I have an alternative to outputting the CSS and JSON...

I currently use Webpack and mini-css-extract-plugin, which supports CSS modules. It also allows me to configure the format of the class names to module names using the modules.localIdentName config option. And this actually has a nice side effect of allowing me to know the module names outside of webpack without needing to compile.

So as an example...

I set the modules.localIdentName to 'cssm_[md5:hash:hex:8]'. Then from this input:

.classOne {
  color: red;
}

I get output like this:

.cssm_abcd1234 {
  color: red;
}

All as you would expect. But of course, in order to actually find out what the resulting module names are, I have to run webpack to get the JSON output of class names to module names.

I have a better way :)

Because I know how the module names are created, i can actually guess what they are without needing to run webpack.

So in Ruby (my language of choice), I can get the module name:

css_path = 'path/to/my/css_modules.css'
class_name = 'classOne'
Digest::MD5.hexdigest("#{css_path}\x00#{class_name}")[0, 8]

I now know the module name without needing to compile webpack and reading the JSON.

Sorry for the slightly long winded description, but what I'm getting at, is could we have a deterministic way of doing the same with parcel-css? This way, all I would need from parcel-css is the CSS output.

I certainly don't need as many config options as mini-css-extract-plugin has. In fact, I don't need any.

Thoughts?

rrcobb added 11 commits January 20, 2022 14:37

refactor minify to a bool option, input file as required positional arg

54bec9c

add option to enable css nesting

d64d05d

enable nesting and css modules

45fe888

partial implementation of css modules feature

3b18f76

infer css modules output path from the normal output path

beb1012

add css modules file option

d66adfe

nicen docstrings

92bacb0

partial impl integration tests

67ee69f

integration tests

5b00e72

rustfmt

5cb5085

add sourcemap option to cli

750b582

rrcobb force-pushed the cli-features branch from 4a03dbd to 750b582 Compare January 20, 2022 19:47

rrcobb commented Jan 20, 2022

View reviewed changes

devongovett merged commit d13f86a into parcel-bundler:master Jan 22, 2022

rrcobb deleted the cli-features branch January 22, 2022 18:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sourcemap, nesting, and css-modules CLI flags #68

Add sourcemap, nesting, and css-modules CLI flags #68

rrcobb commented Jan 20, 2022

height bot commented Jan 20, 2022

rrcobb Jan 20, 2022

devongovett Jan 22, 2022

rrcobb Jan 20, 2022

devongovett Jan 22, 2022

rrcobb Jan 22, 2022

rrcobb Jan 20, 2022

rrcobb Jan 20, 2022

devongovett Jan 21, 2022

rrcobb Jan 22, 2022

rrcobb Jan 20, 2022

rrcobb Jan 20, 2022

devongovett commented Jan 21, 2022

joelmoss commented Jan 21, 2022

devongovett commented Jan 21, 2022

joelmoss commented Jan 21, 2022 •

edited

devongovett commented Jan 22, 2022

rrcobb commented Jan 22, 2022

rrcobb commented Jan 22, 2022

devongovett commented Jan 22, 2022 •

edited

joelmoss commented Jan 22, 2022

joelmoss commented Feb 1, 2022 •

edited

Add sourcemap, nesting, and css-modules CLI flags #68

Add sourcemap, nesting, and css-modules CLI flags #68

Conversation

rrcobb commented Jan 20, 2022

height bot commented Jan 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devongovett commented Jan 21, 2022

joelmoss commented Jan 21, 2022

devongovett commented Jan 21, 2022

joelmoss commented Jan 21, 2022 • edited

devongovett commented Jan 22, 2022

rrcobb commented Jan 22, 2022

rrcobb commented Jan 22, 2022

devongovett commented Jan 22, 2022 • edited

joelmoss commented Jan 22, 2022

joelmoss commented Feb 1, 2022 • edited

joelmoss commented Jan 21, 2022 •

edited

devongovett commented Jan 22, 2022 •

edited

joelmoss commented Feb 1, 2022 •

edited