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

Split Less and SCSS parsing into different parsers #2844

Merged
merged 6 commits into from
Sep 26, 2017

Conversation

lydell
Copy link
Member

@lydell lydell commented Sep 16, 2017

Now, .less files are always parsed with postcss-less, and .scss files
areare always parsed with postcss-scss. This:

parser: "postcss" and --parser postcss continue to work like before:
First trying postcss-less, and if that fails, postcss-scss, unless a
regex says that we should try in the opposite order. The new values for
the parser option are "postcss-less" and "postcss-scss".

Now, .less files are always parsed with postcss-less, and .scss files
areare always parsed with postcss-scss. This:

- Is less hacky.
- Is meant to avoid issues like prettier#2829.
- Is probably more performant.

`parser: "postcss"` and `--parser postcss` continue to work like before:
First trying postcss-less, and if that fails, postcss-scss, unless a
regex says that we should try in the opposite order. The new values for
the parser option are "postcss-less" and "postcss-scss".
@azz
Copy link
Member

azz commented Sep 16, 2017

I feel like the parser option is a bit of a leaky abstraction. It would be nice if the option was called language or something like that so you can pass --language scss and we'd figure out which parser to use.

parser language
babylon javascript
flow flow
typescript (actually typescript-eslint-parser) typescript
graphql graphql
postcss css
postcss-scss scss
postcss-less less
parse5 html
json (actually babylon.parseExpression) json

Unfortunately changing this would obviously be an API breaking change.

@lydell
Copy link
Member Author

lydell commented Sep 16, 2017

I agree that the parser option is strange and could be better. But I don't think it should affect this PR in any way.

Btw, the postcss parts of your table should look more like this:

parser language
postcss (actually postcss-less+postcss-scss+postcss-media-query-parser+postcss-selector-parser+postcss-values-parser) css
postcss-scss (actually like css but minus postcss-less) scss
postcss-less (actually like css but minus postcss-scss) less

@vjeux
Copy link
Contributor

vjeux commented Sep 16, 2017

Agreed, parser was designed before we intended to support more than just pure JavaScript.

I think that it makes sense to introduce a language option and we figure out a way to make it backwards compatible.

@@ -359,7 +359,7 @@
</div>
<div class="options last">
<label>--trailing-comma <select id="trailingComma"><option value="none">none</option><option value="es5">es5</option><option value="all">all</option></select></label>
<label>--parser <select id="parser"><option value="babylon">babylon</option><option value="flow">flow</option><option value="typescript">typescript</option><option value="postcss">postcss</option><option value="json">json</option><option value="graphql">graphql</option></select></label>
<label>--parser <select id="parser"><option value="babylon">babylon</option><option value="flow">flow</option><option value="typescript">typescript</option><option value="postcss">postcss</option><!-- TODO: Enable these when prettier@>1.7.0 has been released. <option value="postcss-less">postcss-less</option><option value="postcss-scss">postcss-scss</option>--><option value="json">json</option><option value="graphql">graphql</option></select></label>
Copy link
Contributor

Choose a reason for hiding this comment

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

1.8.0 now

Copy link
Member

Choose a reason for hiding this comment

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

Exclusive >1.7.0 is technically correct 😉

@@ -84,7 +84,7 @@ Format options:

--no-bracket-spacing Do not print spaces between brackets.
--jsx-bracket-same-line Put > on the last line instead of at a new line.
--parser <flow|babylon|typescript|postcss|json|graphql>
--parser <flow|babylon|typescript|postcss|postcss-less|postcss-scss|json|graphql>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: let's just name it less and scss. And allow both postcss and css to work and only mention css in the docs.

We're unlikely going to ever support anything else than postcss to parse our CSS and at the end of the day the users don't care, they just want to parse their css/less/sass.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean that it is unlikely that we'll support two different parsers for the same language, like we do for JS with both Babylon and Flow? Or that it is unlikely that we'll every switch from postcss to something else?

I have been researching switching away from postcss:

Copy link
Member

Choose a reason for hiding this comment

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

If we did switch away, we probably wouldn't want to keep supporting postcss (another advantage of using --language)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, what I mean is that we just name it with the language, so we don't get stuck to a specific parser if we want to switch.

@azz
Copy link
Member

azz commented Sep 16, 2017

But I don't think it should affect this PR in any way.

Indeed. Opened #2846 for discussion.

Conflicts:
	src/cli-constant.js
	tests_integration/__tests__/__snapshots__/early-exit.js.snap
@lydell lydell mentioned this pull request Sep 25, 2017
@lydell
Copy link
Member Author

lydell commented Sep 25, 2017

Updates:

  • postcss-less → less
  • postcss-scss → scss
  • deprecated postcss in favor of css

@@ -238,6 +238,8 @@ function replaceHash(hash) {

function getCodemirrorMode(options) {
switch (options.parser) {
// TODO: Remove the "postcss" case when prettier@>1.7.0 is released.
Copy link
Member

Choose a reason for hiding this comment

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

Is this a prerelease task, or a postrelease task? (Or doesn't matter)

Copy link
Member Author

Choose a reason for hiding this comment

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

post

@lydell lydell merged commit c6bac7c into prettier:master Sep 26, 2017
@lydell lydell deleted the separate-less-scss-parsers branch September 26, 2017 05:08
@lydell lydell mentioned this pull request Sep 30, 2017
8 tasks
samouri pushed a commit to Automattic/wp-prettier that referenced this pull request Oct 2, 2017
* Split Less and SCSS parsing into different parsers

Now, .less files are always parsed with postcss-less, and .scss files
areare always parsed with postcss-scss. This:

- Is less hacky.
- Is meant to avoid issues like prettier#2829.
- Is probably more performant.

`parser: "postcss"` and `--parser postcss` continue to work like before:
First trying postcss-less, and if that fails, postcss-scss, unless a
regex says that we should try in the opposite order. The new values for
the parser option are "postcss-less" and "postcss-scss".

* Remove postcss from package.json since it is not used

* Rename parser-less to less and parser-scss to scss

* Deprecate parser:postcss in favor of parser:css

* Fix CSS tests
@ismail-syed
Copy link

@lydell As per your comment: deprecated postcss in favor of css
Do the parser docs need updating?

@lydell
Copy link
Member Author

lydell commented Nov 7, 2017

@ismail-syed They do, well spotted!

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 19, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants