-
Notifications
You must be signed in to change notification settings - Fork 47
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
Cleanup template declarations #156
Conversation
@goto-bus-stop I'm gonna add TypeScript support right after this. |
Reworked with proper variable declaration cleanup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks nice overall! Thanks for working on it and I'm sorry it took a long time to get to reviewing.
I think the hardcoded offsets (the -6 and +1 you commented on) are potentially brittle. If some other transform is in between Babel and sheetify those offsets may not be quite right.
I think a better approach is to remove a range from the start of one declarator until the start of the next declarator. That'll even work for weird things like var a = 0, /* lol */ c = 2
because the c
declarator's range only starts at the letter c
. I implemented something similar in static-module: https://github.com/browserify/static-module/blob/08da9185f26b586dd4e83de1c7a0a514d2980e2a/index.js#L75-L96
Everything else LGTM!
@goto-bus-stop Should be better now. |
@goto-bus-stop Build failed due to recent update in third-parties: https://github.com/chalk/supports-color/blame/master/index.js#L5 We should move to Node 6 probably? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excellent work, thanks!
since it's only a dev dependency, i don't think we need to drop node 6, which would require a major version bump. i'll see if we can pin the dependency to an older version somehow that will work on node 4.
We should nullify template literal declarations in order to be properly minified (by purifycss for example). So the following Babel/Buble-transpiled code:
Will be: