Skip to content
This repository has been archived by the owner on Oct 28, 2022. It is now read-only.

Questions #1

Open
iam4x opened this issue Nov 9, 2015 · 8 comments
Open

Questions #1

iam4x opened this issue Nov 9, 2015 · 8 comments

Comments

@iam4x
Copy link
Contributor

iam4x commented Nov 9, 2015

Hey thank's again @IngwiePhoenix :)

I have a few questions:

  1. https://github.com/DragonsInn/bird3-purifycss-webpack-plugin/blob/master/index.js#L34-L48 If I understand well, this will look into webpack compiled files automatically. So do we still need to throw an error when no paths are given (https://github.com/DragonsInn/bird3-purifycss-webpack-plugin/blob/master/index.js#L16) ?
  2. https://github.com/DragonsInn/bird3-purifycss-webpack-plugin/blob/master/index.js#L28 I think we can drop merge dependancy and use Array.concat instead. Same here (https://github.com/DragonsInn/bird3-purifycss-webpack-plugin/blob/master/index.js#L22).
  3. https://github.com/DragonsInn/bird3-purifycss-webpack-plugin/blob/master/index.js#L39 I think we should use a regex like for webpack loaders it would be more understandable for people, keeps consistency in the webpack community.

I will be more than pleased to drop you PR, just need to check these things with you before 👍

@IngwiePhoenix
Copy link
Member

  1. Actually, really good point. o.o Throwing if no paths are specified seems rather dumb if someone builds an app that litterally is made out of nothing but JavaScript. Will remove that in a little bit.
  2. Oh, that exists? Huh - never seen it before. I'll give it a run and see if it does what I want.
  3. My goal was to use a list of extensions here. But if you have a more "webpacky" way to allow both, strings and RegExp'es, feel free to PR that.

I will go and fix Nr. 1 right away in .4

@IngwiePhoenix
Copy link
Member

Tested Array.concat. Seems to do what I want. Working that one out.

@iam4x
Copy link
Contributor Author

iam4x commented Nov 9, 2015

Good I'll drop you PR :)

FYI:

for(var i=0; i<self.scanForExts.length; i++) {
  if(self.scanForExts[i] == ext) {
    isScannable = true;
    break;
  }
}

can be easily refactored into var isScannable = self.scanForExts.includes(ext);
(see: http://devdocs.io/javascript/global_objects/array/includes)

My bad, it's an ES7 proposal you will need to run your code into babel before. What you can do is:

var isScannable = (self.scanForExts.indexOf(ext)  > -1)

@IngwiePhoenix
Copy link
Member

I see. Well, I didn't know of some of the niceties that JS had! But it'd be nice if you could PR that 3rd point. I just implemented 1 and 2.

@iam4x
Copy link
Contributor Author

iam4x commented Nov 9, 2015

@IngwiePhoenix sure I'll take car of that ;)

@IngwiePhoenix
Copy link
Member

Allright. I'll be up for quite a while from hereon out, so i might catch it as you post it. :)

@IngwiePhoenix
Copy link
Member

Done, #2 is merged. :)
Now just for the scanForExts case. Feel free to PR the ability to use RegExp's here as well. :)

@IngwiePhoenix
Copy link
Member

#3 is merged. Oops, actually I should've thought about this beforehand; because if the user would also add JS into the list, it would be possibly duplicated. So, that made quite a lot of sense, it's why I merged it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants