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

Unexpectedly ignored words in @import #88

Closed
lydell opened this issue Aug 21, 2017 · 10 comments · Fixed by #113
Closed

Unexpectedly ignored words in @import #88

lydell opened this issue Aug 21, 2017 · 10 comments · Fixed by #113

Comments

@lydell
Copy link

lydell commented Aug 21, 2017

  • Node Version: 8.3.0
  • NPM Version: 5.3.0
  • postcss-less Version: 1.1.0

LESS

@import missing "missing" "not missing";

JavaScript

require("postcss-less").parse('@import missing "missing" "not missing";')

Expected Behavior

Error thrown, or missing and "missing" being somewhere on the Import node.

Note: The example code is invalid Less ("malformed import statement").

Actual Behavior

The example Less code parses the same as this:

@import "not missing";

In other words, missing and "missing" are totally ignored.

How can we reproduce the behavior?

require("postcss-less").parse('@import missing "missing" "not missing";')

Background

This causes problems for Prettier when people forget the semicolon after the @import: prettier/prettier#2584

@import "../assets/font/league-gothic/league-gothic.css"

.ManagerPage {
  height: 100%;
}

... is turned into this by Prettier:

@import .ManagerPage {
  height: 100%;
}

... because "../assets/font/league-gothic/league-gothic.css" is entirely missing from the AST. Prettier would need:

  • Either an error to be thrown
  • or "../assets/font/league-gothic/league-gothic.css" being somewhere on the Import node so that it can be printed back.
@shellscape
Copy link
Owner

hm that's a bummer. in these kinds of situations it's helpful to have an example of the AST if they AST isn't correct, or is missing things. perfectly acceptable and encouraged to provide that via gist or inline if it's not too bananas.

so we need a few questions answered here:

  1. what's the AST look like in the broken example?
  2. the spec suggests that the semicolon is required, but afaik LESS doesn't require it. is that the case or does LESS actually require a semicolon?
  3. the spec suggests that anything goes after the @import but before the ;. So how do we represent that in an AST?
  4. does postcss proper handle importers in a better way, and can we piggy back on that?

@lydell
Copy link
Author

lydell commented Aug 22, 2017

Here's the AST from postcss-less:

Root {
  raws: { semicolon: false, after: '' },
  type: 'root',
  nodes:
   [ Import {
       raws: { before: '', afterName: ' ', after: ' ', semicolon: true },
       type: 'import',
       nodes: [],
       name: 'import',
       parent: [Circular],
       source:
        { start: { line: 1, column: 1 },
          input:
           Input {
             css: '@import missing "missing" "not missing";',
             id: '<input css 1>' },
          end: { line: 1, column: 40 } },
       importPath: '"not missing"',
       directives: '' } ],
  source:
   { input:
      Input {
        css: '@import missing "missing" "not missing";',
        id: '<input css 1>' },
     start: { line: 1, column: 1 } } }

AFAIK, the semicolon is required in CSS, SCSS and Less. But none of the postcss parsers throw errors for missing semicolons. But the semicolon is a bit of a red herring for this issue.

Also, both postcss and postcss-scss parse the following as a single at-rule with sub-nodes (just like postcss-less):

@import "../assets/font/league-gothic/league-gothic.css"

.ManagerPage {
  height: 100%;
}

So how do we represent that in an AST?

That's what we're here to discuss :)


Both postcss and postcss-scss parse like this:

Root {
  raws: { semicolon: true, after: '' },
  type: 'root',
  nodes:
   [ AtRule {
       raws: { before: '', between: '', afterName: ' ' },
       type: 'atrule',
       name: 'import',
       parent: [Circular],
       source:
        { start: { line: 1, column: 1 },
          input:
           Input {
             css: '@import missing "missing" "not missing";',
             id: '<input css 5>' },
          end: { line: 1, column: 40 } },
       params: 'missing "missing" "not missing"' } ],
  source:
   { input:
      Input {
        css: '@import missing "missing" "not missing";',
        id: '<input css 5>' },
     start: { line: 1, column: 1 } } }

In other words, they don't do anything special for @imports – it's parsed as a general at-rule.


(Also, while I have your attention: Would you mind taking a look at shellscape/postcss-values-parser#36? That's another thing needed for Prettier :) )

@shellscape
Copy link
Owner

Interesting. I could see this being a valuable addition to postcss once complete.

I'm open to suggestions as to how to represent the other bits that are being dropped from the Node. We should follow the spec as closely as possible, while making adjustments for LESS (with things such as directives, which don't exist in the spec)

@import [ <url> | <string> ] <media-query-list>? ;

@lydell
Copy link
Author

lydell commented Aug 22, 2017

I could see this being a valuable addition to postcss once complete.

What does "this" mean here? :)

@lydell
Copy link
Author

lydell commented Aug 22, 2017

Copying in examples from the spec for convenience:

@import url("fineprint.css") print;
@import url("bluish.css") projection, tv;
@import url("narrow.css") handheld and (max-width: 400px);

@shellscape
Copy link
Owner

"this" would be a fully functioning ImportNode that is handled differently than AtRule.

@lydell
Copy link
Author

lydell commented Aug 22, 2017

I not sure that makes sense. As far as I know, postcss does not treat any at-rule in specially, and just stores everything after @foo as one long string. postcss-media-query-parser already seems to parse url("narrow.css") handheld and (max-width: 400px) really well.

@shellscape
Copy link
Owner

Storing <media-query-list>? in a property for further inspection (by something like postcss-media-query-parser) makes a lot of sense to me.

lydell added a commit to lydell/prettier that referenced this issue Aug 22, 2017
parser-postcss parses `@import` at-rules specially, and unfortunately
buggily. This monkey-patches parser-postcss to parse all at-rules the
same way.

Fixes prettier#2584.

postcss-less bug: shellscape/postcss-less#88
vjeux pushed a commit to prettier/prettier that referenced this issue Aug 22, 2017
parser-postcss parses `@import` at-rules specially, and unfortunately
buggily. This monkey-patches parser-postcss to parse all at-rules the
same way.

Fixes #2584.

postcss-less bug: shellscape/postcss-less#88
@shellscape
Copy link
Owner

@lydell ping. any interest in a PR for this one?

@lydell
Copy link
Author

lydell commented Oct 9, 2017

No, no interest. I wish I had, though.

@shellscape shellscape mentioned this issue Sep 17, 2018
16 tasks
shellscape added a commit that referenced this issue Sep 21, 2018
* chore: fix linting errors

* chore: de-esm

* chore: de-esm, start move to ava

* chore: move to circle

* chore: gulp to npm scripts, housekeeping

* chore: unfuck prev de-esm

* test: get root tests passing

* test: better integration tests

* test: finish ava migration

* chore: clean up package, add code coverage

* chore(ci): fix lint script

* test: ignore postcss-parser-tests until after 7.x update

* chore(ci): add node 6 to circle config

* fix: fixes #88, malformed filenames and missing semicolons in imports

* fix: fixes #89, case insensitive !important

* chore: keep the old parsers around for reference temporarily

* refactor: leverage postcss 7.0

* refactor: enable mixins

* chore: improve linting, fix variable parse, enable sanity check tests

* chore: get stringifying working

* chore: re-implement stringifying, update tests, remove old /lib

* test: add test for #110

* test: add test for #108

* chore: implement interpolation, update tests

* fix: fixes #102 and #86

* chore: code cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants