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

css border shorthand incorrectly minified in production #35219

Closed
1 task done
stefanprobst opened this issue Mar 10, 2022 · 4 comments · Fixed by Timer/cssnano-preset-simple#25 or #35258
Closed
1 task done

css border shorthand incorrectly minified in production #35219

stefanprobst opened this issue Mar 10, 2022 · 4 comments · Fixed by Timer/cssnano-preset-simple#25 or #35258
Labels
Upstream Related to using Next.js with a third-party dependency. (e.g., React, UI/icon libraries, etc.).

Comments

@stefanprobst
Copy link
Contributor

Verify canary release

  • I verified that the issue exists in Next.js canary release

Provide environment information

Operating System:
Platform: linux
Arch: x64
Version: #33~20.04.1-Ubuntu SMP Mon Feb 7 14:25:10 UTC 2022
Binaries:
Node: 16.14.0
npm: 8.3.1
Yarn: 1.22.17
pnpm: 6.30.0
Relevant packages:
next: 12.1.1-canary.9
react: 17.0.2
react-dom: 17.0.2

What browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

Describe the Bug

currently, the css minifier (i think this is the compiled-in cssnano-simple) produces invalid css from the following:

h1 {
  --border-width: 8px 2px;

  border-width: var(--border-width);
  border-style: solid;
  border-color: hotpink;
}

results in:

h1{--border-width:8px 2px;border:var(--border-width) solid hotpink}

which is invalid.

note that this is currently also handled incorrectly in upstream cssnano with the default preset, but works correctly with the "lite" preset. cf. cssnano/cssnano#1354

Expected Behavior

don't collapse border properties into shorthand when custom properties are encountered

To Reproduce

@stefanprobst stefanprobst added the bug Issue was opened via the bug report template. label Mar 10, 2022
@wwwdepot
Copy link

It's an optimization enabled by cssnano, See mergeLonghand https://cssnano.co/docs/what-are-optimisations/.

@balazsorban44
Copy link
Member

balazsorban44 commented Mar 11, 2022

Next.js is using a custom variant of the cssnano-preset-default preset, source here: https://github.com/Timer/cssnano-preset-simple

Adding mergeLonghand: false to that and re-compile it for Next.js here:

return postcss([cssnanoSimple({}, postcss)])

might fix the issue, but it looks like cssnano acknowledges this as a bug and we should probably wait for a proper fix on their side: cssnano/cssnano#675 🤔 (admittedly though, that issue has been opened for 3 years now).

Another downside would be that it could affect most current CSS outputs negatively, so the workaround has a cost.

@balazsorban44 balazsorban44 added the Upstream Related to using Next.js with a third-party dependency. (e.g., React, UI/icon libraries, etc.). label Mar 11, 2022
@stefanprobst
Copy link
Contributor Author

there is now a fix for this issue in upstream cssnano v5.1.3

@balazsorban44 balazsorban44 removed the bug Issue was opened via the bug report template. label Mar 11, 2022
@balazsorban44 balazsorban44 reopened this Mar 11, 2022
@kodiakhq kodiakhq bot closed this as completed in #35258 Mar 11, 2022
kodiakhq bot pushed a commit that referenced this issue Mar 11, 2022
Fixes #35219

## Bug

- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Errors have helpful link attached, see `contributing.md`

## Feature

- [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have helpful link attached, see `contributing.md`

## Documentation / Examples

- [ ] Make sure the linting passes by running `yarn lint`
@github-actions
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Upstream Related to using Next.js with a third-party dependency. (e.g., React, UI/icon libraries, etc.).
Projects
None yet
3 participants