Skip to content

fix: Function with dot inside name #134

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

Merged
merged 1 commit into from
May 11, 2021

Conversation

niksy
Copy link
Contributor

@niksy niksy commented Apr 2, 2021

If function contains dot inside it’s name, parser hangs indefinitely (similar to #112). This is possible if Sass Modules namespace is used.

I don’t know if this should be fixed by default or we should add new option where we allow specific symbols for function names (similar to interpolation option).

This PR contains:

  • bugfix
  • feature
  • refactor
  • tests
  • documentation
  • metadata

Breaking Changes?

  • yes
  • no

@shellscape
Copy link
Owner

@niksy this looks good. Please update your snapshots by merging from upstream/master and running npm run test -- --u. Once that's done I can merge this.

@niksy niksy force-pushed the function-dot-separator branch from f852fa1 to d02db41 Compare April 12, 2021 06:17
@niksy
Copy link
Contributor Author

niksy commented Apr 12, 2021

@shellscape OK, done, but now I got a lot of other updated files, is that OK?

@shellscape
Copy link
Owner

shellscape commented Apr 12, 2021

Looks like you need to merge from upstream/master and reinstall dependencies because 0357c82 was merged before you updated your snapshots

@niksy niksy force-pushed the function-dot-separator branch from d02db41 to 6dd49be Compare April 12, 2021 12:36
@niksy
Copy link
Contributor Author

niksy commented Apr 12, 2021

@shellscape yup, forgot to install new dependencies, now it looks OK!

@jonathantneal
Copy link
Collaborator

Fixing the hang is important. Capturing symbols as identifiers would break compatibility with CSS. Is there a way to fix the former without introducing the later?

@shellscape
Copy link
Owner

@jonathantneal any chance you could concoct a test that would show the breakage that @niksy could work with to get an ideal solution?

@dalbitresb12
Copy link

I'm not sure if totally related (and I'm willing to open a separate issue if this is not related), but I've found that the parser also hangs indefinitely if this chunk of CSS is passed:

.leaflet-oldie .leaflet-popup-tip {
  filter: progid:DXImageTransform.Microsoft.Matrix(M11=0.70710678, M12=0.70710678, M21=-0.70710678, M22=0.70710678);
}

The previous CSS block was modified from here.

I've found this bug while trying to use postcss-env-function (which internally uses this library) while importing Leaflet CSS into a Next.js project. While debugging this error in an isolated environment (only with postcss-env-function and the chunk above), I noticed that this while loop never ends with the previous CSS chunk:

parse() {
let token;
while (!this.tokenizer.endOfFile()) {
token = this.tokenizer.nextToken();

The script I used to test it was this one:

const postcss = require('postcss');
const postcssEnvFunction = require('postcss-env-function');

const plugins = [postcssEnvFunction()];

const css = `
.leaflet-oldie .leaflet-popup-tip {
  filter: progid:DXImageTransform.Microsoft.Matrix(M11=0.70710678, M12=0.70710678, M21=-0.70710678, M22=0.70710678);
}
`;

postcss(plugins)
  .process(css)
  .then(result => {
    console.log(result.css);
  });

This will also hang the parser (in the same while loop as before):

const { parse } = require('postcss-values-parser');

const css = 'progid:DXImageTransform.Microsoft.Matrix(M11=0.70710678, M12=0.70710678, M21=-0.70710678, M22=0.70710678)';
const ast = parse(css);
console.log(ast.toString());

As I said, I'm not sure if this issue is related to the one this PR tries to address. If it is not related, I can open another issue.

@shellscape
Copy link
Owner

@jonathantneal checking back in on my last comment. Is that something you're able to do? If not, we can merge this as-is.

@jonathantneal
Copy link
Collaborator

Patching Func#fromTokens or Func#test so that it does not swallow up the full stop (.) opens up other issues, therefore, I think you should merge this as-is.

@shellscape shellscape merged commit 90de00b into shellscape:master May 11, 2021
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 this pull request may close these issues.

4 participants