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

chore(deps)!: upgrade svgo to v3 #1440

Merged
merged 8 commits into from Mar 14, 2023
Merged

chore(deps)!: upgrade svgo to v3 #1440

merged 8 commits into from Mar 14, 2023

Conversation

dargmuesli
Copy link
Contributor

Summary

Upgrade dependency svgo to v3 and increase the minimum supported node version to v14.

Motivation

Keep up to date with the svgo upstream library.

Does this pull request introduce a breaking change?

  • Yes
  • No

Checklist

Documentation

Please check if any of these apply:

  • My proposed change require an update to the documentation
  • I have added or updated the relevant documentation

@dargmuesli
Copy link
Contributor Author

This PR is meant to get work started to support svgo v3 and thus in draft state. I'm not sure what else needs to be done as I'm new to this library's source. Feel free to add or suggest necessary changes. I'm open to improvement suggestions!

tsconfig.json Outdated Show resolved Hide resolved
@CharleeWa
Copy link

CharleeWa commented Oct 24, 2022

Hi, @dargmuesli

Thank you for this PR, I found the questions in the Nuxtjs.

When I add some lib on the Nuxt projects, The command line print this warning:

stable has been deprecated.

So, If this PR is merged, Can I give a PR to Nuxt.js?

Thank you so much!

@dargmuesli
Copy link
Contributor Author

Hey @CharleeWa, thank you for your support.
I'm afraid, I don't know what "questions" and "some lib" is referring to. I also don't know where the warning originates from. Would you create a minimal reproduction?

Regarding Nuxt: cssnano is a subdependency of @nuxt/vite-builder, which is a subdependency of nuxt. If this PR is merged as a breaking change, another PR would be required for @nuxt/vite-builder.

Copy link
Collaborator

@ludofischer ludofischer left a comment

Choose a reason for hiding this comment

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

Thank you for your PR! I think the only issue is that we would need to release a major version of postcss-svgo, cssnano, cssnano-preset-default and cssnano-preset-advanced. Updating to a major version can be inconvenient for those users who install cssnano as a transitive dependency. I've done a quick check :

  1. css-minimizer-webpack-plugin (4371000 downloads) already requires Node.js 14.15, but the most downloaded version is still the 3.x which only requires Node.js 12.x
  2. Parcel: only requires Node.js 12.x but I think they use lightningcss by default
  3. vue-cli: Only requires Node.js 12.x, but is deprecated

What do you think @alexander-akait ? Can we drop Node < 14 and release cssnano 6 in the near future? If we release 6 I would at least also get rid of the YAML parser, since it's only necessary for the (udocumented) YAML configuration feature which I think nobody is using (if they do they can just turn it into JSON)

tsconfig.json Outdated Show resolved Hide resolved
packages/postcss-svgo/package.json Outdated Show resolved Hide resolved
@dargmuesli dargmuesli marked this pull request as ready for review October 24, 2022 11:19
@ludofischer
Copy link
Collaborator

The postcss-svgo tests need to be updated because the result is slightly different but it looks equivalent (different attribute order and minified identifiers)

@alexander-akait
Copy link
Member

@ludofischer I think it is time to drop old nodes, so I am fine with the next v6

@CharleeWa
Copy link

@dargmuesli

Sorry, I wasn't specific enough. If you use the pnpm package manager, the installation dependency will have this prompt. Of course it's not a big deal, just a warning, but it doesn't look very comfortable. So, I think we should wait for cssnano to release a new version, nuxt will update cssnano's version, and the problem will not exist.

截屏2022-10-25 13 58 01

@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2022

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (c09dc45) 97.62% compared to head (c723695) 97.62%.

❗ Current head c723695 differs from pull request most recent head a3ccd12. Consider uploading reports for the commit a3ccd12 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1440      +/-   ##
==========================================
- Coverage   97.62%   97.62%   -0.01%     
==========================================
  Files         122      122              
  Lines       10010    10007       -3     
==========================================
- Hits         9772     9769       -3     
  Misses        238      238              
Impacted Files Coverage Δ
packages/postcss-svgo/src/index.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ludofischer
Copy link
Collaborator

Thank you for updating the tests. It might take a bit longer before we release 6.x New cssnano releases take a long time to reach users, so I would like to do at least another 5.x release with the updates we've just merged on master. Maybe it's also possible to solve some merge-rules bugs on 5.x before we switch to 6.
I'm also thinking we could use the 6 release to rewrite the configuration loading and parsing (see #1436 for an example where the current behaviour is a bit unintuitive).

@dargmuesli
Copy link
Contributor Author

I updated to v3.0.1 and reverted the skipLibCheck addition.

@alexander-akait
Copy link
Member

@dargmuesli can you rebase to solve pnpm merge

dargmuesli and others added 5 commits November 15, 2022 22:58
BREAKING CHANGE: increase the minimum supported node version to v14.
Co-authored-by: Ludovico Fischer <43557+ludofischer@users.noreply.github.com>
@dargmuesli
Copy link
Contributor Author

Rebased as requested ✅

@ludofischer ludofischer added this to the 6.0.0 milestone Nov 20, 2022
@zdm
Copy link

zdm commented Nov 27, 2022

When this PR will be merged?

@silkfire
Copy link

silkfire commented Jan 8, 2023

Bump, when can we merge this PR? @ludofischer

@mahnunchik
Copy link

I'm looking forward to have it merged

@dargmuesli
Copy link
Contributor Author

Please upvote the PR by giving a 👍 on the first comment here and do not add "me too" comments, thanks!

@zdm
Copy link

zdm commented Feb 24, 2023

Oh, is is possible to merge it finally and publish on npm?
PLEASE.h

Copy link

@VIKTORVAV99 VIKTORVAV99 left a comment

Choose a reason for hiding this comment

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

SVGO has released a new patch version so the csso types package should no longer be needed.

packages/postcss-svgo/package.json Outdated Show resolved Hide resolved
packages/postcss-svgo/package.json Outdated Show resolved Hide resolved
@eddiemonge
Copy link

Considering Node14 is almost out of maintenance schedule, getting this in sooner would be much appreciated. I get moving slow with major updates, but considering users do not have to upgrade unless they want to, doing a major update shouldn't take so long or be so painful.

@alexander-akait
Copy link
Member

@ludofischer Maybe it is time to do major release and drop old nodejs support?

@ludofischer
Copy link
Collaborator

What features would you want from SVGO 3? There seem to be at least one unsolved regression from SVGO 2: svg/svgo#1746. I don't know how impactful these are though.
I was hoping to find the time to improve the configuration reading as I think some configuration options right now have no effect (as that would be another breaking change), but obviously it seems I never manage to get around to doing it. So we might as well release cssnano 6.0 just with SVGO bump, dropping the YAML support and possibly dropping normalize-url.

Copy link
Collaborator

@ludofischer ludofischer left a comment

Choose a reason for hiding this comment

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

I think we can merge this.

@ludofischer ludofischer merged commit 4e272f8 into cssnano:master Mar 14, 2023
@dargmuesli dargmuesli deleted the chore/deps/scgo branch March 15, 2023 03:29
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.

None yet