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

Initial dead code elimination #20

Closed
wants to merge 2 commits into from
Closed

Initial dead code elimination #20

wants to merge 2 commits into from

Conversation

rtsao
Copy link
Contributor

@rtsao rtsao commented Aug 22, 2015

Implemented dead code elimination and updated tests. Fixes #13

I could be missing something in terms of the greater context including css-modulesify/css-modules-require-hook but for the sake of this repo, dead code elimination now works. It is also possible I'm doing something naive, but as far as I can tell everything works.

I basically removed the caching that was previously happening, I probably can trivially put back in some caching in the case of composing the same classes from the same file (but not in the case of different classes from the same file). We probably could also make dead code elimination a configuration option and thus optional if you're worried about compile performance (for example during hot loading).

I intend to clean up the tests a little bit and add some more tests (including comprehensive unit tests for the containsClass function).

This PR contains lint/formatting problems which I intend to fix after #19 is addressed.

@rtsao rtsao mentioned this pull request Aug 22, 2015
@rtsao
Copy link
Contributor Author

rtsao commented Aug 27, 2015

@geelen @joshwnj

@joshwnj
Copy link
Member

joshwnj commented Aug 27, 2015

cool stuff @rtsao ! Will take a closer look and try with css-modulesify, but so far sounds great.

@geelen
Copy link
Member

geelen commented Aug 31, 2015

My apologies, I hadn't had a proper chance to look at this yet. Am doing so now.

@joshwnj
Copy link
Member

joshwnj commented Sep 7, 2015

Hey @rtsao sorry for the delay, it's been a busy couple of weeks :) So far from what I can see this is looking good. The only issue I've spotted so far is that animation keyframes aren't being included as expected. I'm seeing this in http://css-modules.github.io/browserify-demo/ if you want to try to repro.

@BinahLinden
Copy link

Is this a dead issue? I am also looking for a solution to this

@geelen
Copy link
Member

geelen commented Aug 16, 2016

@BinahLinden I'm afraid so. We're hoping to support this again at some point but it's a bigger job than this PR indicates.

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.

Dead code elimination
4 participants