search

royriojas / esformatter-jsx

esformatter plugin: format jsx files (or js files with Facebook React JSX Syntax)
MIT License
142 stars 25 forks source link

Inline `falafel`. #91

Closed wtgtybhertgeghgtwtg closed 7 years ago

wtgtybhertgeghgtwtg commented 7 years ago

See comment. It's very rough, but how does it look? Would it be considered?

royriojas commented 7 years ago

hi @wtgtybhertgeghgtwtg thanks for your contribution. I would like understand better why this is needed. While I do like the idea of inlining falafel I would like to consider also changing the node version to at least node 4. As shown in http://node.green/ that version won't need isArray nor Object.keys. Just make sure tests are passing and will merge in order to remove deps.

wtgtybhertgeghgtwtg commented 7 years ago

It is not really a need, but it would make things simpler. You wouldn't need to maintain fresh-falafel, for one, and you wouldn't have to worry about acorn causing issues with CSP, since it flat out drops the acorn dependency. I have been having trouble with esbeautifier beautifier on my machine, but I will continue to try and resolve that so that I can get the tests passing.

royriojas commented 7 years ago

Thanks @wtgtybhertgeghgtwtg, again thanks for your PR. Looking forward to your changes.

wtgtybhertgeghgtwtg commented 7 years ago

It looks like esbeautifier needs a .esformetter file to work, but I cannot seem to find one for this repository. Am I missing something? Disregard.

wtgtybhertgeghgtwtg commented 7 years ago

Tests pass. Using single quotes in scripts is causing issues on Windows, though.

royriojas commented 7 years ago

@wtgtybhertgeghgtwtg can you please elaborate? what kind of issues?

wtgtybhertgeghgtwtg commented 7 years ago

For example, the beautify-check script will end up passing ["{DIRECTORY}/'index.js'", "{DIRECTORY}/'lib/**/*.js'", "{DIRECTORY}/'specs/**/*.spec.js'"] instead of ["{DIRECTORY}/index.js", "{DIRECTORY}/'lib/**/*.js", "{DIRECTORY}/specs/**/*.spec.js"] to glob-expand.

royriojas commented 7 years ago

@wtgtybhertgeghgtwtg are u sure? I tested this in windows 7 (a VM) and it seems it is working as expected.

royriojas commented 7 years ago

In any case let me investigate about it.

On the other hand... can we get rid of object-keys as a dependency? we need to bump the travis.yml to node 4, and 6, I guess by now is safe to ignore node < 1

wtgtybhertgeghgtwtg commented 7 years ago

object-keys removed.

royriojas commented 7 years ago

Thank you very much @wtgtybhertgeghgtwtg