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

Support nested parentheses in url() functions #84

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nex3
Copy link
Contributor

@nex3 nex3 commented Nov 30, 2021

Although this isn't strictly speaking valid CSS, it's common for
preprocessors and postprocessors to support function calls and similar
constructs within url() delimiters, since they appear to be function
calls themselves.

Closes #34
Closes #46

Although this isn't strictly speaking valid CSS, it's common for
preprocessors and postprocessors to support function calls and similar
constructs within `url()` delimiters, since they appear to be function
calls themselves.

Closes TrySound#34
Closes TrySound#46
@alexander-akait
Copy link
Collaborator

It is breaking change, we should put it under the option

@nex3
Copy link
Contributor Author

nex3 commented Nov 30, 2021

IMO this isn't worth categorizing as a "breaking change"... the old behavior was so weird (creating a value token that didn't have the last trailing parenthesis and then a single dangling ) token) that I think it should be classified as a bug rather than an intended feature that can then be broken. I think it's unlikely anyone was relying on that specifically.

@alexander-akait
Copy link
Collaborator

alexander-akait commented Nov 30, 2021

URL has different logic parsing according CSS spec (url token), nested functions are not allowed, https://www.w3.org/TR/css-syntax-3/#consume-url-token, so it is by design, some packages rely on this behavior, so we can't just change it without additional options

@nex3
Copy link
Contributor Author

nex3 commented Dec 1, 2021

Which packages rely on not being able to parse nested functions?

@alexander-akait
Copy link
Collaborator

css-loader in webpack, stylelint and more

@alexander-akait
Copy link
Collaborator

alexander-akait commented Dec 1, 2021

I am not against this change, I just say that it can lead to some unexpected problems

@nex3
Copy link
Contributor Author

nex3 commented Dec 1, 2021

I checked this change against the tests for stylelint, css-loader, and cssnano as well for good measure, and all of them passed (well, stylelint has a few failures against the current master, but this didn't produce any new failures). I really don't think this will cause any real problems in practice.

@alexander-akait
Copy link
Collaborator

I think it can break edge case - url(url("something.png"))

@nex3
Copy link
Contributor Author

nex3 commented Dec 1, 2021

It still produces a totally reasonable parse there:

{
  type: 'function',
  sourceIndex: 0,
  value: 'url',
  before: '',
  nodes: [
    {
      type: 'word',
      sourceIndex: 4,
      sourceEndIndex: 14,
      value: "url('foo')"
    }
  ],
  after: '',
  sourceEndIndex: 15
}

that should be handled more or less correctly by any of these tools, given that it's invalid CSS in the first place. In fact, it's a more reasonable parse than the old version:

{
  type: 'function',
  sourceIndex: 0,
  value: 'url',
  before: '',
  nodes: [
    {
      type: 'word',
      sourceIndex: 4,
      sourceEndIndex: 13,
      value: "url('foo'"
    }
  ],
  after: '',
  sourceEndIndex: 14
}

@alexander-akait
Copy link
Collaborator

Hm, old output is wrong too, because ( can be used in url token https://www.w3.org/TR/css-syntax-3/#consume-url-token...

Regarding to css-loader, now we try to resolve url('foo' url, after this change we will try to resolve foo, I understand, no one do not use nested url in real code, so in theory it is safe to merge, but current logic and logic there are wrong by spec...

@nex3
Copy link
Contributor Author

nex3 commented Dec 2, 2021

I think there should be some flexibility to do something the user expects when dealing with text that's explicitly invalid according to the CSS specification. Remember that this package isn't just used to parse 100%-spec-compliant CSS, it's also used to implement CSS extensions. Sass for example does allow nested functions within url() (although that's not actually the use-case that caused me to add this).

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.

throw error on invalid url func URL parsing differs from other native functions
2 participants