feat: add node API and cli #227

zamotany · 2018-09-23T12:35:35Z

#214

Let's decide what options we want to support in CLI and over experience, then I'll add the docs.

callstack-bot · 2018-09-23T12:36:18Z

Hey @zamotany, thank you for your pull request 🤗.
The coverage report for this branch can be viewed here.

satya164

I think in a library, I would usually want to:

generate a single CSS file, we should just be able concat all CSS files for this
when generating multiple CSS files, I would want to insert require statements to the JS files so the CSS is actually picked up, we would probably want to accept the directory where the built JS files are, to insert require statements

satya164 · 2018-09-23T12:46:13Z

src/node.js

+    const { css, map } = transform(filename, code, sourceMaps);
+    return {
+      ...output,
+      [filename]: { css, map: map || null },


Also need to set the source content for the source maps so it knows what's in the source JS file. e.g.

linaria/src/loader.js

Line 28 in 62da50e

map.setSourceContent(

satya164 · 2018-09-23T12:48:27Z

bin/linaria.js

+      '',
+      'Options:',
+      '  --help, -h            Show help                    [boolean]',
+      '  --sourceMaps, -m      Generate source maps         [boolean]',


Maybe -s is nicer. Other CLIs such as Babel use -s to denote source maps.

satya164 · 2018-09-23T12:49:45Z

bin/linaria.js

+      'Options:',
+      '  --help, -h            Show help                    [boolean]',
+      '  --sourceMaps, -m      Generate source maps         [boolean]',
+      '  --outDir, -o          Output directory             [string]',


Camelcase isn't common in CLIs, so probably --source-maps and --out-dir is better instead of --sourceMaps and --outDir

satya164 · 2018-09-23T12:53:01Z

bin/linaria.js

+  mkdir.sync(outDir);
+  Object.keys(styles).forEach(filename => {
+    fs.writeFileSync(
+      path.join(outDir, path.basename(filename)),


We would probably also want to preserve the directory structure, otherwise, CSS files may overwrite each other when 2 JS files have the same name

satya164 · 2018-09-23T12:54:15Z

bin/linaria.js

+
+  mkdir.sync(outDir);
+  Object.keys(styles).forEach(filename => {
+    fs.writeFileSync(


We should check if there is any CSS first before writing the file, and if CSS is empty, skip writing

satya164 · 2018-09-23T12:56:23Z

bin/linaria.js

+    if (styles[filename].map) {
+      fs.writeFileSync(
+        path.join(outDir, `${path.basename(filename)}.map`),
+        styles[filename].map


Probably should also add a sourceMapURL in the CSS file.

zamotany · 2018-09-27T15:49:46Z

@thymikee can you check this? any feedback?

thymikee

LGTM but I'm checking on a phone :D

out of date

satya164 · 2018-09-28T10:03:47Z

bin/linaria.js

+
+      fs.writeFileSync(cssOutput, cssContent);
+
+      // Add require to original JS file to import extracted CSS.


We don't want to add require to the original source files. We should accept a path to the directory where the user has built JS files and insert them there.

bin/linaria.js

satya164 · 2018-09-28T10:10:00Z

bin/linaria.js

+      .replace(path.extname(jsFilename), '.css');
+    const cssOutput = path.join(outDir, folderStructure, cssFilename);
+
+    // Skip writing if the file already exists and the new CSS code is empty.


Not sure if we should skip writing if CSS file exists. If I change something in the JS and run it again, I'd expect the CSS files to be updated. If we skip it, they'll never be updated unless you delete the existing files first manually.

Please ready carefully: Skip writing if the file already exists and the new CSS code is empty

It was done per your request:

We should check if there is any CSS first before writing the file, and if CSS is empty, skip writing

I didn't mean if the file already exists, I said if there is any CSS i.e. if there's any CSS text.

satya164 · 2018-09-28T10:11:05Z

bin/linaria.js

+    const cssOutput = path.join(outDir, folderStructure, cssFilename);
+
+    // Skip writing if the file already exists and the new CSS code is empty.
+    if (!fs.existsSync(cssOutput) || styles[jsFilename].css) {


Maybe instead of checking styles[jsFilename].css here, the node API should avoid adding the file into the result if there's no CSS

bin/linaria.js

satya164 · 2018-09-28T10:14:21Z

src/node.js

+    }
+    return {
+      ...output,
+      [filename]: { css, map: map || null },


Probably should return map.toString() instead of the generator instance

satya164 · 2018-09-28T10:15:16Z

bin/linaria.js

+      '  --config, -c          Path to Babel config file         string]',
+      '',
+      'Example:',
+      '  linaria -m ./file1.js ./file2.js',


This needs to be updated

satya164 · 2018-09-28T10:15:33Z

bin/linaria.js

+      '  --source-maps, -s     Generate source maps             [boolean]',
+      '  --require-css, -r     Require CSS in original JS file  [boolean]',
+      '  --out-dir, -o         Output directory                 [string]',
+      '  --config, -c          Path to Babel config file         string]',


missing brackets?

also should mention which options are required and throw if not provided. I'd recommend using something like yargs to make it easier instead of writing it manually https://github.com/yargs/yargs

None of the options are required. Only arguments are.

@zamotany --out-dir should be required, no? otherwise would be too easy to pollute working directory with css files by mistake.

It would be, I think babel for example doesn't require you to provide out dir. Or maybe we should set some default like build? or css_dist?

maybe extracted_css or something. but maybe better just to require it. I'd want to change it anyways.

satya164 · 2018-10-15T10:33:07Z

Would be good to update it per the new transform function. Also maybe we should expose transform for the public API and the cli for working with files?

satya164 · 2018-12-16T00:51:35Z

I rebased the PR against master since there were conflicts.

satya164 · 2019-01-06T02:39:44Z

Hey, I fixed the behavior of inserting require statements (it was supposed to insert them in a given directory with compiled JS files, not the source files).

I also replaced commander with yargs because there was barely any validation of the arguments in commander. yargs also seems easier to use and the parsed arguments are typed, which is a plus.

Please check and merge when you have time :)

src/cli.js

refactor: replace commander with yargs yargs has better validation options and seems easier to use than commander. the parsed args are typed as well, which is a bonus fix: pass options when evaluating a component chore: release 1.0.0-beta.4 Update src/cli.js Co-Authored-By: zamotany <[email protected]> fix typo Co-Authored-By: zamotany <[email protected]> feat: support configuring linaria with a config file The babel preset is used in several ways: - babel config - webpack loader - rollup plugin - stylelint preprocessor Having a config file makes it possible to configure it in one place instead of configuring separately for each tool. Merge branch 'master' into feat/node-api-and-cli

satya164 previously requested changes Sep 23, 2018

View reviewed changes

zamotany requested a review from thymikee September 27, 2018 15:49

thymikee approved these changes Sep 27, 2018

View reviewed changes

zamotany requested a review from satya164 September 28, 2018 09:21

satya164 reviewed Sep 28, 2018

View reviewed changes

satya164 mentioned this pull request Sep 28, 2018

Linaria 1.0 roadmap #214

Closed

12 tasks

satya164 mentioned this pull request Nov 7, 2018

Support and document how to publish modules using Linaria to NPM #178

Open

2 tasks

satya164 force-pushed the feat/node-api-and-cli branch 4 times, most recently from 9e2e71a to 296d0c9 Compare December 16, 2018 00:50

satya164 force-pushed the feat/node-api-and-cli branch 2 times, most recently from 45523b7 to fd4af7b Compare January 6, 2019 02:36

zamotany commented Jan 7, 2019

View reviewed changes

src/cli.js Outdated Show resolved Hide resolved

satya164 reviewed Jan 7, 2019

View reviewed changes

src/cli.js Outdated Show resolved Hide resolved

zamotany force-pushed the feat/node-api-and-cli branch from 371fff8 to fed3aa4 Compare January 7, 2019 12:20

zamotany and others added 3 commits January 7, 2019 13:22

Merge branch 'master' into feat/node-api-and-cli

e8d89fd

fix

2665b34

Update preprocessor.js

24e7e31

satya164 merged commit 8ace1f3 into master Jan 7, 2019

satya164 deleted the feat/node-api-and-cli branch January 7, 2019 12:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add node API and cli #227

feat: add node API and cli #227

zamotany commented Sep 23, 2018

callstack-bot commented Sep 23, 2018 •

edited

Loading

satya164 left a comment

satya164 Sep 23, 2018

satya164 Sep 23, 2018

satya164 Sep 23, 2018

satya164 Sep 23, 2018

satya164 Sep 23, 2018

satya164 Sep 23, 2018

zamotany commented Sep 27, 2018

thymikee left a comment

satya164 Sep 28, 2018

satya164 Sep 28, 2018

zamotany Sep 28, 2018

satya164 Sep 28, 2018

zamotany Sep 28, 2018

satya164 Sep 28, 2018

satya164 Sep 28, 2018

satya164 Sep 28, 2018

satya164 Sep 28, 2018

satya164 Sep 28, 2018

zamotany Sep 28, 2018

satya164 Sep 28, 2018

zamotany Sep 28, 2018

satya164 Sep 28, 2018

satya164 commented Oct 15, 2018

satya164 commented Dec 16, 2018

satya164 commented Jan 6, 2019


		fs.writeFileSync(cssOutput, cssContent);

		// Add require to original JS file to import extracted CSS.

feat: add node API and cli #227

feat: add node API and cli #227

Conversation

zamotany commented Sep 23, 2018

callstack-bot commented Sep 23, 2018 • edited Loading

satya164 left a 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

zamotany commented Sep 27, 2018

thymikee left a 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

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

satya164 commented Oct 15, 2018

satya164 commented Dec 16, 2018

satya164 commented Jan 6, 2019

callstack-bot commented Sep 23, 2018 •

edited

Loading