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

Refactor to remove autoprefixer dependency #5312

Merged
merged 3 commits into from
May 19, 2021
Merged

Refactor to remove autoprefixer dependency #5312

merged 3 commits into from
May 19, 2021

Conversation

ybiquitous
Copy link
Member

@ybiquitous ybiquitous commented May 17, 2021

This change removes the autoprefixer dependency (also its type definitions), and changes the logic of the isAutoprefixable utility.

Which issue, if any, is this issue related to?

Closes #5293
Closes #5019
Closes #4971

Is there anything in the PR that needs further explanation?

This PR removes autoprefixer, but I've created some prefix lists by using the internals of autoprefixer.

This change removes the `autoprefixer` dependency (also its type definitions),
and change the logic of the `isAutoprefixable` utility.
@ybiquitous ybiquitous linked an issue May 17, 2021 that may be closed by this pull request
@ybiquitous
Copy link
Member Author

The following documents mentions autoprefixer and they may be inaccurate after merging this PR.
But I don't have a good idea how to change the descriptions. Any help?

return false;
}

return PROPERTIES.has(vendor.unprefixed(ident));
Copy link
Member Author

Choose a reason for hiding this comment

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

[note] This property() logic change requires the fix of lib/rules/property-no-vendor-prefix/__tests__/index.js.
Because autoprefixer can fix -ms-interpolation-mode to image-rendering, but this new simple implementation cannot support such a difficult case. I think the limitation is acceptable.

See also https://developer.mozilla.org/en-US/docs/Web/CSS/image-rendering#examples

Copy link
Member Author

Choose a reason for hiding this comment

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

I reconsidered. I think it acceptable to just adding a -ms-interpolation-mode hack (bd87465).

@ybiquitous ybiquitous marked this pull request as ready for review May 17, 2021 18:54
Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

Fantastic, thank you!

This PR removes autoprefixer, but I've created some prefix lists by using the internals of autoprefixer.

SGTM. These lists are rarely going to change, if at all, because browsers now favour feature flags over prefixes.

The following documents mentions autoprefixer and they may be inaccurate after merging this PR. But I don't have a good idea how to change the descriptions. Any help?

I think we can strip them right back:

"This rule ignores non-standard vendor-prefixed properties/selectors/etc that aren't handled by Autoprefixer."

Replacing "properties/selectors/etc" with whatever is appropriate for the rule.

This "This rule ignores" format will be consistent with the extended description in other rules.

Did you want to update the docs in this pull request?

@jeddy3 jeddy3 changed the title Remove autoprefixer dependency Refactor to remove autoprefixer dependency May 18, 2021
@ybiquitous
Copy link
Member Author

@jeddy3 Thank you for the feedback! 😄

Did you want to update the docs in this pull request?

No. It is best if we don’t need to change them.

@jeddy3
Copy link
Member

jeddy3 commented May 18, 2021

No. It is best if we don’t need to change them.

Sure thing. We can change them in follow-up PR to:

"This rule ignores non-standard vendor-prefixed properties/selectors/etc that aren't handled by Autoprefixer."

(Replacing "properties/selectors/etc" with whatever is appropriate for the rule.)

@ybiquitous ybiquitous linked an issue May 18, 2021 that may be closed by this pull request
@jeddy3 jeddy3 merged commit 6086255 into v14 May 19, 2021
@jeddy3 jeddy3 deleted the remove-autoprefixer branch May 19, 2021 13:52
@ybiquitous
Copy link
Member Author

@jeddy3 Thank you for the documentation fix! So nice. 👍🏼

'-o-radial-gradient',
'-o-repeating-linear-gradient',
'-o-repeating-radial-gradient',
'-webkit- oldlinear-gradient',
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the property values with space inside OK?

Copy link
Member Author

Choose a reason for hiding this comment

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

@victor-homyakov This list is output by Autoprefixer. Here is the snippet to produce the list:

package.json:

{
  "dependencies": {
    "autoprefixer": "10.2.5"
  }
}

index.js:

const autoprefixer = require("autoprefixer");
const Browsers = require("autoprefixer/lib/browsers");
const Prefixes = require("autoprefixer/lib/prefixes");
const prefixes = new Prefixes(
  autoprefixer.data.prefixes,
  new Browsers(autoprefixer.data.browsers, [])
);

const list = Object.values(prefixes.remove)
  .filter((p) => Array.isArray(p.values))
  .flatMap((p) => p.values)
  .map((p) => p.prefixed);
console.log(new Set(list.sort()));

Run:

$ docker run --rm -v $PWD:/work -w /work node:16.2.0 sh -c 'npm install && node index.js'

up to date, audited 14 packages in 2s

4 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities
Set(60) {
  '-moz-available',
  '-moz-box',
  '-moz-calc',
  '-moz-crisp-edges',
  '-moz-element',
  '-moz-fit-content',
  '-moz-grab',
  '-moz-grabbing',
  '-moz-inline-box',
  '-moz-isolate',
  '-moz-isolate-override',
  '-moz-linear-gradient',
  '-moz-max-content',
  '-moz-min-content',
  '-moz-plaintext',
  '-moz-radial-gradient',
  '-moz-repeating-linear-gradient',
  '-moz-repeating-radial-gradient',
  '-moz-zoom-in',
  '-moz-zoom-out',
  '-ms-flexbox',
  '-ms-grid',
  '-ms-inline-flexbox',
  '-ms-inline-grid',
  '-ms-linear-gradient',
  '-ms-radial-gradient',
  '-ms-repeating-linear-gradient',
  '-ms-repeating-radial-gradient',
  '-o-linear-gradient',
  '-o-pixelated',
  '-o-radial-gradient',
  '-o-repeating-linear-gradient',
  '-o-repeating-radial-gradient',
  '-webkit- oldlinear-gradient',
  '-webkit- oldradial-gradient',
  '-webkit- oldrepeating-linear-gradient',
  '-webkit- oldrepeating-radial-gradient',
  '-webkit-box',
  '-webkit-calc',
  '-webkit-cross-fade',
  '-webkit-fill-available',
  '-webkit-filter',
  '-webkit-fit-content',
  '-webkit-flex',
  '-webkit-grab',
  '-webkit-grabbing',
  '-webkit-image-set',
  '-webkit-inline-box',
  '-webkit-inline-flex',
  '-webkit-isolate',
  '-webkit-linear-gradient',
  '-webkit-max-content',
  '-webkit-min-content',
  '-webkit-optimize-contrast',
  '-webkit-radial-gradient',
  '-webkit-repeating-linear-gradient',
  '-webkit-repeating-radial-gradient',
  '-webkit-sticky',
  '-webkit-zoom-in',
  '-webkit-zoom-out'
}

I don't know why Autoprefixer outputs a whitespace-included property such as -webkit- oldradial-gradient.
But I think this will no be a problem in most cases because such properties are outdated.

Do you have some idea to handle them?

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked into the code of autoprefixer.

It marks with ' old' cases with more than one possible syntax, e.g. '-webkit- old', '-ms- old', at least here and here.

Then such prefixes are handled in a special way

  • here the utility function removes ' old' part from prefix
  • here and here are separate code paths for such prefixes

I think that for purposes of this PR you can use utility function, which removes ' old' from vendor prefix, i.e. you'll end up with '-webkit-linear-gradient' (valid) instead of '-webkit- oldlinear-gradient' (invalid).

Copy link
Member Author

Choose a reason for hiding this comment

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

@victor-homyakov Thank you so much for the investigation! 👏🏼

Are you interested in creating a new PR to fix the invalid properties?
(If no, I'll create it instead.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to create a PR, but I'm very busy next two weeks

Copy link
Member Author

Choose a reason for hiding this comment

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

@victor-homyakov No problems. I've created PR #5321.
Thank you so much for your investigation! 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Refactor *-no-vendor-prefix to avoid using autoprefixer Fix slow autoprefixer calls in *-no-vendor-prefix
3 participants