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

Recover reject css with webpack plugin #819

Closed

Conversation

Quidam74
Copy link

@Quidam74 Quidam74 commented Dec 17, 2021

Proposed changes

Recover purged css in separete css file for webpack plugin.

see #816 and #763.

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

First time for me and my coworker (@Firologame) working on a public repo. Also discovering how webpack plugin work and first contact with typescript. We don't know how to create tests, but our developement didn't create any error on the existing tests. Please be gentle !

Feel free to upgrade those few changes and give us feedback, thank you for your attention !

@kevinramharak
Copy link
Contributor

The rejectedCss feature is already being tested by its own tests. I'll take a look and test it out. You can add a case in this array. I'll take a shot at it if I find the time.

Copy link
Contributor

@kevinramharak kevinramharak left a comment

Choose a reason for hiding this comment

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

You should also make sure the src/types/index.ts are updated correctly as these will provide proper code completion when using the plugin

Just add the option in the appropriate interface like:

export interface UserDefinedOptions {
 // ...
  output?: string;
  rejected?: boolean;
  rejectedCss?: boolean;
  // ...
}

@@ -136,6 +136,10 @@ new PurgeCSSPlugin({

If `true` all removed selectors are added to the [Stats Data](https://webpack.js.org/api/stats/) as `purged`.

* #### rejectedCss
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how complicated it would be, but I imagine it would be more webpack friendly if it uses their template string configuration options.

So it would allow for something like:

const options = {
  output: '[name].rejected.[ext]'
}

@@ -127,7 +128,17 @@ export default class PurgeCSSPlugin {

// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
compilation.updateAsset(name, new ConcatSource(purged.css));
let source: Source = new ConcatSource(purged.css)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why you are re-using the source variable? I would suggest something more in the original style like:

compilation.updateAsset(name, new ConcatSource(purged.css));
if (purged.rejectedCss !== undefined) {
  const rejectedName = path.dirname(name) + '/' + path.basename(name, '.css') + '-rejected' + path.extname(name);
  const source = new ConcatSource(purged.rejectedCss);
  if (compilation.getAsset(rejectedName) {
    compilation.emitAsset(rejectedName, source);  
  } else {
   compilation.updateAsset(rejectedName, source);
  }
}

Some minor nitpick would be to maintain consistent with use of semi-colons in the file.

@kevinramharak
Copy link
Contributor

@Quidam74 I made a PR to your branch that adds a test case for this feature. I'm going to take a shot at using the TemplatePathPlugin API. If those 2 are merged I think it is good to go.

@kevinramharak
Copy link
Contributor

@Quidam74 Added the webpack template string option in this PR. It would allow the following:

// Output the rejected css as `[base]-rejected.[ext]`
new PurgeCSSPlugin({
  rejectedCss: true,
});
// or use a custom output path
new PurgeCSSPlugin({
  rejectedCss: '[base].rejected.css',
});

@github-actions
Copy link

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Apr 11, 2022
@kevinramharak
Copy link
Contributor

@Quidam74 Is there any interest in closing this issue? If so please review my PR to your fork so we can revisit this PR.

@Quidam74
Copy link
Author

hi there, Sorry for the long time afk.

i don't understand what you want me to do, not used to work with forks.

You want me to merge your pull request on my fork ? in this place : Quidam74#2 ?

@kevinramharak
Copy link
Contributor

@Quidam74 Well I just realized I made a PR on your main branch, and not the feature branch. I will have to change the branch and check the changes first. But after that yes, if you could merge my PR then this feature would be ready for purgecss

@kevinramharak
Copy link
Contributor

@Quidam74 I checked it and it seems good. What it does is add test coverage and allows for naming the output file (with your foo.css -> foo-rejected.css as default). Once its merged I will check the conflicts and poke FullHuman to merge it into purgecss

…lugin-webpackOutputPathTemplate

Recover reject css with webpack plugin webpack output path template
@Quidam74
Copy link
Author

Done ! thank you for your help !

@github-actions github-actions bot removed the Stale label Apr 12, 2022
@github-actions
Copy link

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Jul 11, 2022
@github-actions github-actions bot closed this Jul 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants