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

upgrade postcss #169

Merged
merged 3 commits into from
May 10, 2021
Merged

upgrade postcss #169

merged 3 commits into from
May 10, 2021

Conversation

TrySound
Copy link
Contributor

Ref https://github.com/postcss/postcss/releases/tag/8.0.0

Looks like after all we have to drop node 8 support.

@TrySound
Copy link
Contributor Author

cc @bholloway

@TrySound TrySound changed the title Upgrade postcss [v4] Upgrade postcss Sep 15, 2020
@TrySound TrySound changed the title [v4] Upgrade postcss [v4] upgrade postcss Sep 15, 2020
@bholloway
Copy link
Owner

I read "changes to API" and thought the worst, thanks for showing this change is not too painful. 👍

Although I am not sure how to time this. V4 is overdue and it would be nice to ship something compatible with node 8 simply for all those poor souls stuck with old projects.

That said I think keeping on top of postcss updates is a good thing so it would be a quick followup if delayed.

@TrySound
Copy link
Contributor Author

Yeah, probably v4 can be released with just adjust loader upgrade and then v5 with new schema utils and postcss.

@bholloway
Copy link
Owner

Webpack 5 requires 10 as minimum and just looking at the v5 project it already has some related tasks

  • bump node to >=10.
  • drop support for Webpack 2 and Webpack 3

So I will add this to v5.

@bholloway bholloway changed the title [v4] upgrade postcss [v5] upgrade postcss Oct 20, 2020
@ArnaudLier
Copy link

Is there any workaround atm?

@TrySound
Copy link
Contributor Author

@ZeProf2Code Why? Do you have problem with postcss 7?

@ArnaudLier
Copy link

I want to upgrade to TailwindCSS 2 and I need PostCSS 8.

@TrySound
Copy link
Contributor Author

AFAIK in this package postcss is used only internally. It should not affect upgrade on your side.

@brian-kephart
Copy link

I'm having the same difficulty as @ZeProf2Code when attempting to upgrade PostCSS to version 8. Webpack gets a ton of compile errors pointing back to this package. It seems like PostCSS 7/8 don't coexist peacefully in this context.

@TrySound
Copy link
Contributor Author

TrySound commented Dec 2, 2020

Which errors? Might be unrelated problem.

@dvdknaap
Copy link

Is there any update because more and more packages are requiring postcss 8 ?

@dvdknaap
Copy link

dvdknaap commented Mar 17, 2021

@bholloway when i force the version 4 and postcss 8 everything is working on my end

yarn add resolve-url-loader@4.0.0-alpha.3 --dev
yarn add postcss@^8.0.0 --dev

edit: without the postcss force you still get the error that postcss 8 is required for fontawesome so i guess it's still dependancy issue

@bholloway
Copy link
Owner

bholloway commented Mar 25, 2021

@dvdknaap I've been thinking more on this.

I could probably put a semver comparitor inthat allows postcss@8.

"postcss": ">=7.0.35 <9",

I'm not sure that it would drop support for node 8 per sec, since anyone on node 8 could still use postcss@7 right?. That would allow us to address the issue in V4.

Does that sound like it would work? I mean, the work done by @TrySound is awesome but AFAIK we should be able to use postcss@8 for a while with the old API right?

EDIT - This doesn't work as I would have hoped. The engines check will not bump the installed version down, it will just fail install. 😞

@bholloway bholloway changed the title [v5] upgrade postcss upgrade postcss Mar 25, 2021
@jmfrancois
Copy link

jmfrancois commented Apr 21, 2021

It could also be a peer dependencies ? so the project decide the version it uses and you try to ensure compatiblity with as many versions as you want on your side ?

On our side we need to upgrade to postcss 8 because of security fix provided in last release (8.2.10) and this is the last dependency I have which rely on postcss 7 without ugprade option (except force resolution)

Fixed ReDoS vulnerabilities in source map parsing.

@ErideonTech
Copy link

https://nvd.nist.gov/vuln/detail/CVE-2021-23368

resolve-url-loader is a package used by Angular (compiler only), hence flags as a security issue for us
Thanks for updating

@bholloway
Copy link
Owner

bholloway commented Apr 29, 2021

Okay let me see what we can do.

Please lets move to the pinned discussion issue to discuss further.
EDIT - my mistake there is no discussion issue.
EDIT - #198 is now the discussion issue.

In the mean time please consider the workaround @dvdknaap suggested above.

@bholloway bholloway mentioned this pull request May 1, 2021
@bholloway bholloway changed the base branch from master to v5-development May 10, 2021 03:31
@bholloway
Copy link
Owner

For some reason this is failing hard in E2E tests. As soon as I work out the issue I will merge the PR. If there is any additional work I will do that on the v5 branch rather than asking for more changes here.

@TrySound et. al. FYI for v5 I intend to require node@>=12 and to push all the dependencies to the latest.

function eachDeclaration(declaration) {
const postcssPlugin = {
postcssPlugin: 'postcss-resolve-url',
Declaration: (declaration) => {
Copy link
Owner

Choose a reason for hiding this comment

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

This code has been locking up E2E tests. The test eventually crashes Node with "out of memory" error.

Finally worked out from this migration guide that...

plugins will re-visit all changed or added nodes. You should check if your transformations were already applied and if that is the case—ignore those nodes.

So here the code needs to somehow keep track of visited declarations.

() => {
  postcssPlugin: 'postcss-resolve-url',
  prepare: () => {
    const visited = new Set();
    return {
      Declaration: (declaration) => {
        ...
      }
    };
  }
};

I will fix this and make some other tweaks post-merge.

@bholloway bholloway merged commit e094fa6 into bholloway:v5 May 10, 2021
@bholloway bholloway mentioned this pull request May 20, 2021
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.

7 participants