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

cssnano not playing nice with font-display #420

Closed
WDD-Marv opened this Issue Oct 31, 2017 · 29 comments

Comments

Projects
None yet
5 participants
@WDD-Marv

WDD-Marv commented Oct 31, 2017

In my postcss config I've got font-magician configured to add font-display: optional;, but there seems to be an issue with cssnano 3.x/4.x;

after getting errors & hoping that cssnano@next would clear it up, I've manually added all the plugins & selectively disabled them until no errors occurred:

[
  require('postcss-colormin')(),
  /*
  require('postcss-convert-values')(),
  */
  require('postcss-discard-comments')(),
  require('postcss-discard-duplicates')(),
  require('postcss-discard-empty')(),
  require('postcss-discard-overridden')(),
  /*
  require('postcss-discard-unused')(),
  */
  require('postcss-merge-idents')(),
  require('postcss-merge-longhand')(),
  require('postcss-merge-rules')(),
  require('postcss-minify-font-values')(),
  require('postcss-minify-gradients')(),
  require('postcss-minify-params')(),
  require('postcss-minify-selectors')(),
  require('postcss-normalize-charset')(),
  require('postcss-normalize-display-values')(),
  require('postcss-normalize-positions')(),
  require('postcss-normalize-repeat-style')(),
  /*
  require('postcss-normalize-string')(),
  */
  require('postcss-normalize-timing-functions')(),
  require('postcss-normalize-unicode')(),
  /*
  require('postcss-normalize-url')(),
  require('postcss-normalize-whitespace')(),
  */
  require('postcss-ordered-values')(),
  require('postcss-reduce-idents')(),
  require('postcss-reduce-initial')(),
  require('postcss-reduce-transforms')(),
  require('postcss-svgo')(),
  require('postcss-unique-selectors')(),
  require('postcss-zindex')(),
]

the errors that occur are either TypeError: value.charCodeAt is not a function or TypeError: node.value.replace is not a function and only occur if the @font-family blocks have a font-display declaration present, i.e. font-display: optional ;

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Apr 19, 2018

@andyjansson very strange, we don't have font-display nowhere
@WDD-Marv friendly ping, problem still exists, can you provide more information?

WDD-Marv added a commit to WDD-Marv/cssnano-420 that referenced this issue May 3, 2018

@WDD-Marv

This comment has been minimized.

WDD-Marv commented May 3, 2018

@evilebottnawi basic reproduction repo posted; issue occurs if either cssnano or cssnano@next is used, issue does not occur with just postcss-font-magician.

@kuflash

This comment has been minimized.

Contributor

kuflash commented Jun 2, 2018

I'm tried debugging this problem and found strange thing. Perhaps the problem is still in the postcss-font-magician, because if we put simple css with @font-face rule with font-display property into cssnano, it will work correctly and compress everything. But if we put postcss-font-magician into our pipeline of plugins, we catch exception.

Perhaps this is due to this conversion of options transmitted to postcss-font-magician. Because into end we get decl.value = ['options'], but on idea should receive simply decl.value = 'options'.

Maybe I'm wrong, then correct me.

@WDD-Marv

This comment has been minimized.

WDD-Marv commented Jun 4, 2018

@kuflash the pull request you filed over at jonathantneal/postcss-font-magician#65 does indeed fix the issue with both cssnano and cssnanonext see repo with patch manually applied :D

@kuflash

This comment has been minimized.

Contributor

kuflash commented Jun 4, 2018

@WDD-Marv Yes, I also checked my fix on your repository. It helped a lot. Waiting for when they will accept PR)

@WDD-Marv

This comment has been minimized.

WDD-Marv commented Jun 4, 2018

@kuflash do you happen to know where in the postcss docs is the part that describes the input/output formats that plugins are intended to support ?

@kuflash

This comment has been minimized.

Contributor

kuflash commented Jun 4, 2018

@WDD-Marv now I'm trying to find this information. I need to somehow get the AST output to check the validity of the decl.value.

@WDD-Marv

This comment has been minimized.

WDD-Marv commented Jun 4, 2018

@kuflash i'm thinking less about AST (code) and more about what it's meant to do (spec), i.e. is the plugin linked to in the postcss docs behaving correctly when it doesn't expect an array as input ?

@kuflash

This comment has been minimized.

Contributor

kuflash commented Jun 4, 2018

@WDD-Marv good point. I'm will try use it's.

@kuflash

This comment has been minimized.

Contributor

kuflash commented Jun 5, 2018

@WDD-Marv my PR was accepted. I think you need to wait for the new release postcss-font-magician and you can close this issue.

@WDD-Marv

This comment has been minimized.

WDD-Marv commented Jun 5, 2018

seems to work tentatively, although as you say, it's best to wait for new release :)

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 5, 2018

@WDD-Marv @kuflash can you provide css what reproduce error?

@WDD-Marv

This comment has been minimized.

WDD-Marv commented Jun 5, 2018

@evilebottnawi please refer to this repo

@kuflash

This comment has been minimized.

Contributor

kuflash commented Jun 5, 2018

@evilebottnawi the problem was not in the css, but in the fact that in decl.value passed the array, although the documentation there is expected to string.

Apparently, when the postcss is already doing the last stage of processing, then it normalized it, but when transmitting the result to another plug-in an error occurred.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 5, 2018

@kuflash hm, do you known who do it (plugin from cssnano)?

@WDD-Marv

This comment has been minimized.

WDD-Marv commented Jun 5, 2018

@evilebottnawi it's the postcss-value-parser plugin receiving an array sent from postcss-font-magician when font magician is configured to use font-display in the @font-face declaration.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 5, 2018

@WDD-Marv in theory we should not modify standard postcss properties/fields for nodes for save compatibility with other plugins

@WDD-Marv

This comment has been minimized.

WDD-Marv commented Jun 5, 2018

@evilebottnawi this is why i was wondering if postcss-value-parser was behaving according to spec in not accepting array values.

@WDD-Marv

This comment has been minimized.

WDD-Marv commented Jun 5, 2018

p.s.; back to the original bug report, the bug was only occurring when certain components of cssnano were in use.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 5, 2018

Somebody can send PR with tests to ensure bug will be fixed or exists?

@WDD-Marv

This comment has been minimized.

WDD-Marv commented Jun 5, 2018

@evilebottnawi to postcss-value-parser ?

@evilebottnawi

This comment has been minimized.

@kuflash

This comment has been minimized.

Contributor

kuflash commented Jun 5, 2018

I can create PR with test by this example.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 5, 2018

@kuflash looks like no problems with test 😕

@kuflash

This comment has been minimized.

Contributor

kuflash commented Jun 5, 2018

We not pass param display in fontMagician. If you pass it, you get an error. For example

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 5, 2018

@kuflash can you change test your test to failing?

@kuflash

This comment has been minimized.

Contributor

kuflash commented Jun 5, 2018

@evilebottnawi sure. Now I will prepare an PR with this test.

kuflash added a commit to kuflash/cssnano that referenced this issue Jun 5, 2018

evilebottnawi added a commit that referenced this issue Jun 6, 2018

tests: add test for checking compatibility with postcss-font-magician (
…#504)

* add test for checking issue #420

* upgrade postcss-font-magician to 2.2.0

* fix version rule for postcss-font-magician

* fix yarn.lock
@WDD-Marv

This comment has been minimized.

WDD-Marv commented Jun 6, 2018

:D

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 6, 2018

Any helping with other bugs welcome 👍

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