WIP: Add PostCSS's SugarSS recognition by default #1689

DoumanAsh · 2018-07-09T08:04:12Z

Personally I find SugarSS is pretty nice simplified version of CSS syntax.

Since its addition is configured by setting corresponding parse in PostCSS's config, I wonder if it would be ok to add its extension by default?

Kompwu · 2018-07-09T17:11:44Z

@DoumanAsh Can you help me ?
I'm trying to get .sss bundled...

DoumanAsh · 2018-07-09T17:18:59Z

@Kompwu Well as far as I understand currently you'd need to register this extension to be recognized as CSS asset:

const Bundler = require('parcel-bundler');

let bundler = new Bundler('input.js');
bundler.addAssetType('.sss', './assets/CSSAsset');

If I understand this API correctly(I'm currently decided to use plain CSS with nesting support

Kompwu · 2018-07-09T18:03:54Z

@DoumanAsh Thanks,
How can i make the same thing without js ?
Edit: @DeMoorJasper Can you help me ?

DoumanAsh · 2018-07-09T18:31:19Z

@Kompwu Sorry I'm not sure as I'm new to parcel myself

Kompwu · 2018-07-10T12:41:24Z

@DeMoorJasper Can you merge it pls ?

DeMoorJasper · 2018-07-10T14:36:40Z

It would be nice if the sugarSS asset actually installed and configured postCSS for the user, this way it works zero-config, you can probably create an asset that runs sss through postCSS and then passes it to css. (Hope this makes any sense, lemme know if you need any help)

DoumanAsh · 2018-07-11T05:41:36Z

@DeMoorJasper Ok, I'll try to approach with this suggestion. But what should we do if user already has config?

DeMoorJasper · 2018-07-11T16:54:48Z

@DoumanAsh You can probably load the PostCSS config and check for SugarSS and just forward it to CSS if it's already installed and configured I guess?

Kompwu

You should load the PostCSS configuration and check for SugarSS & transpile it to CSS if it's already installed and configured.
If not, configure PostCSS & SugarSS & transpile it to CSS.

DoumanAsh · 2018-07-15T09:01:35Z

I tried to give it a go by extending existing CSS asset, but it makes me think that it is rather difficult to do.

The problem is not only to load postcss config, but there is also parse method that requires you to specify syntax so I'd need to load sugarss in two places it seems

Kompwu · 2018-07-15T15:10:35Z

@DoumanAsh Do what you can and we'll help you with everything else.

Kompwu · 2018-07-17T11:51:19Z

@DoumanAsh How's going?

DoumanAsh · 2018-07-17T12:05:56Z

@Kompwu Sorry, I've been a bit busy lately, I'll try to resume working on it as soon as I have time

DoumanAsh · 2018-07-17T14:42:07Z

Currently I have issue with PostCSS API usage as it seems to cause errors when I attempt to parse .sss:
```CssSyntaxError: Missed semicolon````

Though I did set let root = postcss.parse(code, {from: this.name, to: this.name, syntax: sugarss}) corresponding parser.

I was thinking that I could parse SugarSSS into CSS and then rely on existing CSSAsset but it seems I misunderstood the API as it ignores provided by me syntax.

Kompwu · 2018-07-29T18:49:38Z

@DoumanAsh Any news ?
@devongovett Hey, can you help us ?

DoumanAsh · 2018-07-29T19:17:16Z

@Kompwu Sorry, recently I'm busy with other things and didn't look into PostCSS

Kompwu · 2018-08-06T09:23:50Z

@DoumanAsh How's going ?

DoumanAsh · 2018-08-06T09:38:57Z

@Kompwu Sorry I'm not actively using parcel as of the moment and kinda busy so I'm not planning to resume work on it until need arise again.

I don't mind someone to carry on, or leave issue open so that me or someone else could continue later on.

Kompwu · 2018-08-09T09:00:28Z

@ai Hi, can you take a look ?

ai · 2018-08-09T14:17:05Z

src/assets/SSSAsset.js

+  }
+
+  parse(code) {
+    let root = postcss.parse(code, {from: this.name, to: this.name, syntax: sugarss});


syntax => parser.

We need to set SugarSS only as input language (parser) and keep output language (stringifier) as default CSS

I think I also tried it initially, but for some reason postcss still parsed code as if it was CSS

I will try to run tests after Def Con

I can try if you want.

Yeap, it should be parser:.

So all that remains is to automate the configuration?

DeMoorJasper

Just locking this for merge untill this is finished

Kompwu · 2018-08-18T16:40:43Z

@DeMoorJasper Are you sure automating configuration is a good thing?
I don't think Parcel should do it.

DeMoorJasper · 2018-08-18T16:49:18Z

@Kompwu We are not automating config, we're adding a default?

I just thought it might be useful to actually have something with extension sugarSS be transpiled to css by default (it should still read the config and potentially even just pass along the sugarss file to postCSS without any transforms if it's configured). The rest of the PostCSS transforms will still apply as far as I know...

Kompwu · 2018-08-19T13:10:41Z

@DoumanAsh Can you finish?
All that remains is to add a default configuration if there is none.

DoumanAsh · 2018-08-19T14:34:44Z

@Kompwu To be honest I'm not sure why, but even when I place parser, it still fails in simple integration test.

The current commit also makes sugarss required dependency, would it be possible to make it optional?

DeMoorJasper · 2018-08-19T18:51:34Z

How does it add it as a dependency, you could use localRequire, that's how it's supposed to be used.

DeMoorJasper · 2018-08-19T18:52:21Z

src/assets/SSSAsset.js

+  }
+
+  parse(code) {
+    let root = postcss.parse(code, {from: this.name, to: this.name, parser: sugarss});


let sugarss = await localRequire('sugarss')

DeMoorJasper · 2018-08-19T18:52:30Z

src/assets/SSSAsset.js

@@ -0,0 +1,35 @@
+const postcss = require('postcss');
+const sugarss = require('sugarss');


should be a local require and a dev dependency for testing...

DeMoorJasper · 2018-08-19T18:54:15Z

src/assets/SSSAsset.js

+  }
+}
+
+class CSSAst {


Split this up into a seperate file, so both CSS and SugarSS Asset use it

DeMoorJasper · 2018-08-19T18:55:06Z

src/transforms/postcss.js

+  if (config.parser === undefined && asset.options.css_parser !== undefined) {
+      const name = asset.options.css_parser;
+
+      if (!(typeof name === "string")) throw "Internal Error: Invalid PostCSS parser type. Should be string";


typeof name !== "string"

DeMoorJasper · 2018-08-19T18:57:04Z

src/transforms/postcss.js

+
+      if (!(typeof name === "string")) throw "Internal Error: Invalid PostCSS parser type. Should be string";
+
+      let parser = await localRequire(name, asset.name).catch(() => require(name));


why does it catch with a require, that's what localRequire already does?

localRequire's should just throw if it fails, because require is just the same thing. (Unless you want to require something from inside parcel, but that should happen inside the transform not be a fallback as this could have unwanted side-effects)

DeMoorJasper · 2018-08-19T18:57:57Z

src/assets/SSSAsset.js

+
+class SSSAsset extends CSSAsset {
+  constructor(name, options) {
+    options.css_parser = "sugarss";


Why is this necessary?

I wanted to propagate to transform to automatically load parser, if it is necessary.

But not sure if it is good idea

You already do a localRequire in transform so not sure why this is necessary at all.

DoumanAsh · 2018-08-20T17:03:07Z

Thanks for review, I applied majority of comments.

I'm not sure though how to automate configuration for PostCSS though.

My simple integration test is still failing me:

  sugarss
    1) should correctly parse SugarSS asset


  0 passing (478ms)
  1 failing

  1) sugarss
       should correctly parse SugarSS asset:
     CssSyntaxError: Missed semicolon
      at Input.error (node_modules\postcss\lib\input.js:119:22)
      at Parser.checkMissedSemicolon (node_modules\postcss\lib\parser.js:544:26)
      at Parser.decl (node_modules\postcss\lib\parser.js:269:50)
      at Parser.other (node_modules\postcss\lib\parser.js:168:18)
      at Parser.parse (node_modules\postcss\lib\parser.js:84:26)
      at Function.parse (node_modules\postcss\lib\parse.js:24:16)
      at SSSAsset.parse (src\assets\SSSAsset.js:16:24)

DeMoorJasper · 2018-08-20T17:41:23Z

src/assets/Ast.js

@@ -0,0 +1,18 @@
+class CSSAst {


Rename the file to CSSAst, as js and html also have an ast...

I was thinking that it would be better for future to have common source of asts?

In the future every asset will have it's own package so it will be seperate anyways

DeMoorJasper · 2018-08-20T17:42:03Z

src/assets/Ast.js

+  }
+}
+
+module.exports.CSSAst = CSSAst;


Just make this the default export module.exports = CSSAst;

DeMoorJasper · 2018-08-20T17:46:01Z

You could check if the postCSS config already has a sugarss transform setup and skip this transform entirely if it has. But in the test it probably shouldn't have any postCSS config. Otherwise you're basically just testing postCSS

DeMoorJasper · 2018-08-20T18:04:13Z

I've changed some stuff around and it should be working better now. You might want to cleanup and debug it a bit more as it's not actually working right now, and def not clean

DeMoorJasper · 2018-08-26T21:37:16Z

Gonna open up a new PR that bases of off this one, with fixed code

Kompwu mentioned this pull request Jul 9, 2018

SugarSS Support #1692

Closed

Kompwu suggested changes Jul 12, 2018

View reviewed changes

DoumanAsh force-pushed the patch-1 branch from e0298a2 to 4006c41 Compare July 15, 2018 08:59

devongovett added the 🙋‍♀️ Feature label Jul 16, 2018

ai suggested changes Aug 9, 2018

View reviewed changes

DeMoorJasper requested changes Aug 11, 2018

View reviewed changes

DeMoorJasper changed the title ~~Add PostCSS's SugarSS recognition by default~~ WIP: Add PostCSS's SugarSS recognition by default Aug 11, 2018

DoumanAsh force-pushed the patch-1 branch from 4006c41 to e314d2f Compare August 19, 2018 14:32

DeMoorJasper reviewed Aug 19, 2018

View reviewed changes

Add PostCSS's SugarSS recognition by default

3330f1f

DoumanAsh force-pushed the patch-1 branch from e314d2f to 3330f1f Compare August 20, 2018 16:58

DeMoorJasper requested changes Aug 20, 2018

View reviewed changes

DeMoorJasper added 4 commits August 20, 2018 10:47

Update postcss.js

2c0e831

Update SSSAsset.js

ab4ca82

Update Ast.js

1ab7ad0

Update SSSAsset.js

4310363

Update SSSAsset.js

aa4cfa2

DeMoorJasper closed this Aug 26, 2018

DeMoorJasper mentioned this pull request Aug 26, 2018

Sugarss support #1941

Merged

		@@ -0,0 +1,35 @@
		const postcss = require('postcss');
		const sugarss = require('sugarss');


		if (!(typeof name === "string")) throw "Internal Error: Invalid PostCSS parser type. Should be string";

		let parser = await localRequire(name, asset.name).catch(() => require(name));

WIP: Add PostCSS's SugarSS recognition by default #1689

WIP: Add PostCSS's SugarSS recognition by default #1689

Conversation

DoumanAsh commented Jul 9, 2018

Kompwu commented Jul 9, 2018

DoumanAsh commented Jul 9, 2018

Kompwu commented Jul 9, 2018 • edited

DoumanAsh commented Jul 9, 2018

Kompwu commented Jul 10, 2018

DeMoorJasper commented Jul 10, 2018

DoumanAsh commented Jul 11, 2018

DeMoorJasper commented Jul 11, 2018 • edited

Kompwu left a comment

Choose a reason for hiding this comment

DoumanAsh commented Jul 15, 2018

Kompwu commented Jul 15, 2018

Kompwu commented Jul 17, 2018

DoumanAsh commented Jul 17, 2018

DoumanAsh commented Jul 17, 2018

Kompwu commented Jul 29, 2018

DoumanAsh commented Jul 29, 2018

Kompwu commented Aug 6, 2018

DoumanAsh commented Aug 6, 2018

Kompwu commented Aug 9, 2018

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

DeMoorJasper left a comment

Choose a reason for hiding this comment

Kompwu commented Aug 18, 2018

DeMoorJasper commented Aug 18, 2018

Kompwu commented Aug 19, 2018

DoumanAsh commented Aug 19, 2018

DeMoorJasper commented Aug 19, 2018

Choose a reason for hiding this comment

DeMoorJasper Aug 19, 2018 • edited

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

DoumanAsh commented Aug 20, 2018

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

DeMoorJasper commented Aug 20, 2018

DeMoorJasper commented Aug 20, 2018

DeMoorJasper commented Aug 26, 2018

Kompwu commented Jul 9, 2018 •

edited

DeMoorJasper commented Jul 11, 2018 •

edited

DeMoorJasper Aug 19, 2018 •

edited