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

Fix false positives for escaped multi-line URL in string-no-newline #4057

Open
AlexWayfer opened this issue Apr 26, 2019 · 7 comments
Open
Labels
status: ask to implement ask before implementing as may no longer be relevant type: bug a problem with a feature or rule

Comments

@AlexWayfer
Copy link

AlexWayfer commented Apr 26, 2019

Clearly describe the bug

I want to split large strings for aesthetic reasons with enabled string-no-newline rule.

There is the quote from the specs, including:

And also: "It is possible to break strings over several lines, for aesthetic or other reasons, but in such a case the newline itself has to be escaped with a backslash ()."

But for some reason escaped newlines are forbidden in Stylelint.

Which rule, if any, is the bug related to?

string-no-newline

What CSS is needed to reproduce the bug?

background-image: url(
  "data:image/svg+xml;utf8,\
  <svg fill='black' height='24' \
    viewBox='0 0 24 24' width='24' \
    xmlns='http://www.w3.org/2000/svg'\
  ><path d='M7 9l5 5 5-5z'/></svg>"
);

What stylelint configuration is needed to reproduce the bug?

extends: stylelint-config-standard

Which version of stylelint are you using?

9.10.1

How are you running stylelint: CLI, PostCSS plugin, Node.js API?

CLI with stylelint assets/styles/.

Does the bug relate to non-standard syntax (e.g. SCSS, Less etc.)?

No.

What did you expect to happen?

No warnings to be flagged.

What actually happened (e.g. what warnings or errors did you get)?

The following warnings were flagged:

 116:197  ✖  Expected line length to be no more than 80 characters                                               max-line-length
@AlexWayfer AlexWayfer changed the title Allow escaped newlines in string-no-newline Allow escaped newlines with string-no-newline Apr 26, 2019
@hudochenkov
Copy link
Member

Thanks for the report and for using the template.

I can't reproduce this error with the following code:

.hello {
  background-image: url(
    "data:image/svg+xml;utf8,\
    <svg fill='black' height='24' \
      viewBox='0 0 24 24' width='24' \
      xmlns='http://www.w3.org/2000/svg'\
    ><path d='M7 9l5 5 5-5z'/></svg>"
  );
}

Looks like you're also has max-line-length: 80 in your config. I can't reproduce error with this rule enabled as well.

Could you check please what is actual CSS, which triggers warning? Maybe there are many indentation symbols, and line is actually bigger than 80 symbols.

@hudochenkov hudochenkov added the status: needs clarification triage needs clarification from author label Apr 30, 2019
@hudochenkov
Copy link
Member

Looks like our demo is misleading, it doesn't show problem, when CLI shows.

For the following CSS:

.hello {
  background-image: url("data:image/svg+xml;utf8,\
	  <svg fill='black' height='24' \
		viewBox='0 0 24 24' width='24' \
		xmlns='http://www.w3.org/2000/svg'\
	  ><path d='M7 9l5 5 5-5z'/></svg>");
}

I got this violation:

2:51  ✖  Unexpected newline in string   string-no-newline

Looks like it's a bug.

As the issue has been labelled "help wanted", please consider contributing a fix.

P. S. In original issue violation is incorrect, which confused me.

@hudochenkov hudochenkov added status: ready to implement is ready to be worked on by someone type: bug a problem with a feature or rule and removed status: needs clarification triage needs clarification from author labels Apr 30, 2019
@AlexWayfer
Copy link
Author

P. S. In original issue violation is incorrect, which confused me.

Yes, my bad, sorry. There was a mess with #4059. But there're still two problems.

@hudochenkov hudochenkov changed the title Allow escaped newlines with string-no-newline Fix false positives for escaped multi-line URL in string-no-newline Apr 30, 2019
@vankop
Copy link
Member

vankop commented Jul 25, 2019

@hudochenkov Same problem #4059, postcss does not parse data url node:
Steps to reproduce:

  1. lib/rules/max-line-length/tests/index.js
"use strict";

const rule = require("..");
const { messages, ruleName } = rule;

const testUrl = `data:image/svg+xml;utf8, \
      <svg fill='black' height='24' \
        viewBox='0 0 24 24' width='24' \
        xmlns='http://www.w3.org/2000/svg'\
      ><path d='M7 9l5 5 5-5z'/></svg>`;

testRule(rule, {
  ruleName,
  config: [true],

  accept: [
    {
      code: `div { background: "${testUrl}"}`
    },
    {
      code: `div { background: "${encodeURI(testUrl)}"}`
    },
]});
  1. run node --inspect-brk node_modules/jest/bin/jest.js --runInBand lib/rules/string-no-newline/__tests__/index.js

  2. In debugger add breakpoint/debugger; here
    https://github.com/stylelint/stylelint/blob/master/lib/getPostcssResult.js#L134
    In first run result.result.root.nodes: [], in second run result.result.root.nodes: [Node]

@vankop
Copy link
Member

vankop commented Jul 25, 2019

Looks like our demo is misleading, it doesn't show problem, when CLI shows.

For CLI it parses 🙈, but failing after on https://github.com/stylelint/stylelint/blob/master/lib/rules/string-no-newline/index.js#L83 parser returns source string instead of removing \ and newline symbols

Copy link
Contributor

This issue is older than one month. Please ask before opening a pull request, as it may no longer be relevant.

@github-actions github-actions bot added status: ask to implement ask before implementing as may no longer be relevant and removed status: ready to implement is ready to be worked on by someone labels Jan 22, 2024
@Mouvedia
Copy link
Member

related: postcss/postcss#1349

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ask to implement ask before implementing as may no longer be relevant type: bug a problem with a feature or rule
Development

No branches or pull requests

4 participants