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

Webpack #1149

Merged
merged 2 commits into from
May 12, 2017
Merged

Webpack #1149

merged 2 commits into from
May 12, 2017

Conversation

bitwiseman
Copy link
Member

@evocateur @einars @HookyQR @olsonpm @mmsqe

Hello all. This is an early preview of my plan to begin moving to a module based structure for this project. I've like to hear any feedback you have, but there's no hurry, there's still quite a way to go before it lands. If nothing else, I need to mirror the modularization in the Python implementation.

There are two conflicting requirements for this change:

  1. Add the ability to reuse modules
  2. Make this a completely non-breaking change for end users.

I think I've achieved that. I could use any help you can provide

When this goes in, I will let it bake and make further non-breaking internal changes and clean up.
As part of this change I change the css beautifier to use the same Output class as the js beautifier.

I think this and various follow up refactorings will unblock a number of improvements, such as parsing of html and css, fewer bugs, and eventually a 2.0 release the drops a bunch of legacy hacks.

@bitwiseman bitwiseman added this to the v1.7.0 milestone Feb 28, 2017
@bitwiseman bitwiseman force-pushed the webpack branch 12 times, most recently from e32e088 to dc31c89 Compare March 1, 2017 01:15
@bitwiseman bitwiseman closed this Mar 1, 2017
@bitwiseman bitwiseman reopened this Mar 1, 2017
Copy link
Contributor

@evocateur evocateur left a comment

Choose a reason for hiding this comment

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

Nice!


install:
- rm -rf ~/.nvm && git clone https://github.com/creationix/nvm.git ~/.nvm && (cd ~/.nvm && git checkout `git describe --abbrev=0 --tags`) && source ~/.nvm/nvm.sh && nvm install $TRAVIS_NODE_VERSION
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this incantation necessary? I thought Travis already used nvm?

Copy link
Member Author

Choose a reason for hiding this comment

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

@evocateur - only on node projects. This is python project that happens to run node. Travis would default to node 0.12. This was fine until this change - webpack requires a newer version of node to work. Thus incantation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah, I forgot about that.

@bitwiseman bitwiseman closed this Mar 3, 2017
@bitwiseman bitwiseman reopened this Mar 3, 2017
@olsonpm
Copy link
Contributor

olsonpm commented Mar 3, 2017

My two cents

  • The begin/end templates used for the lib output is pretty strange. I then started looking for that same functionality in webpack and quickly gave up as I always do when searching for anything with webpack haha. I did find the banner plugin which looked like it would suffice for begin, and end should be taken care of by the umd output as your comment mentions.

  • Out of curiosity, why did you extract inputscanner and token into their own modules? They are only required by the javascript beautifier, which makes me wonder how you decided what should be extracted as modules.

everything else looks pretty straight forward - and the modularization will definitely provide real value.

I'm sorry to say I can't spend any time testing as I'm knee deep in a couple side projects and haven't used any beautifiers in some time (my employer's CI process is laughable so I've just gotten used to 'Laissez-faire' looking code).

Edit: As always, I respect the time you put into this library @bitwiseman. You do a solid job.

@bitwiseman
Copy link
Member Author

@olsonpm

  • Begin/end Template: I want to have this be functionally equivalent from an api perspective. As you ran into, I couldn't do that using the autogenerated wrapper from webpack. In the long run, I'll remove the templating. It's a hack but it is a hack that is controlled by the project
  • Extracting classes - I plan to use the the classes in common for all three beautifiers. Figured I'd extract more classes now to unblock that work, in case anyone has the inclination to start.

Thanks for taking a look. I appreciate the time you've spent on the project and your continued participation.

@bitwiseman
Copy link
Member Author

@evocateur @olsonpm
Thanks for taking a look and asking questions. I appreciate the time you've spent on the project and your continued participation.

@HookyQR
Copy link
Contributor

HookyQR commented Mar 14, 2017

@bitwiseman Sorry I haven't provided any input. Been pretty busy. May get to have a look this week. 🤞

@bitwiseman bitwiseman merged commit 895a45d into beautifier:master May 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants