cloudfour / core-hbs-helpers

Handlebars helpers that we usually need
MIT License
11 stars 2 forks source link

ESnext? #30

Open tylersticka opened 8 years ago

tylersticka commented 8 years ago

I've been totally spoiled by our other repos. In particular, I think Handlebars helpers would benefit a lot from the new parameter stuff.

But, I understand the value of keeping these things lean as well. There could be performance issues or something similar. I'd consider this a low priority nicety.

erikjung commented 8 years ago

We've been deferring the syntax conversion until we have an eslint config in place, so that we can simply add the config, then rely on Atom's linter to identity all of the syntax updates needed.

The process would look like this:

  1. Create a repo for our shareable config (e.g. eslint-config-cloudfour)
  2. Add an .eslintrc to this repo that simply extends the shared config (extends: cloudfour)
  3. Update files as needed, using https://github.com/AtomLinter/linter-eslint as a validator
erikjung commented 8 years ago

Also, another thing to consider is whether or not we want to rely on Babel. I think it'd be nice to not have to transpile these helpers, since we're only targeting the Node environment for now, and most of the syntax features we want are supported natively in Node 4.0.

tylersticka commented 8 years ago

and most of the syntax features we want are supported natively in Node 4.0.

Ooh! I did not know that. That's cool.

If it ends up being simpler to just keep things ES5 and avoid the dependency, I shan't be upset with that decision either. (FWIW)

erikjung commented 8 years ago

Ooh! I did not know that. That's cool.

Well, to be clearer, the specific things you mentioned (default params, rest, spread) wouldn't be supported, but basic things like arrows, let, const, and object literal enhancements would. We'd also get most of the new built-in types like Map, Set, etc.

Node 4: http://kangax.github.io/compat-table/es6/#node4 Node 5: http://kangax.github.io/compat-table/es6/#node5

We might want to target Node 5 instead, but not all of us are running that yet.

tylersticka commented 8 years ago

Good to know!

(That is one ugly table!)

erikjung commented 8 years ago

@tylersticka @mrgerardorodriguez

This branch contains an ESLint config: https://github.com/cloudfour/core-hbs-helpers/tree/chore-esnextify

With that in place, we can begin addressing this issue by:

gerardo-rodriguez commented 8 years ago

(That is one ugly table!)

LOL, yes it is.

This branch contains an ESLint config

Awesome! What is the plan to roll this out? Are we doing a trial run of sorts?

erikjung commented 8 years ago

Awesome! What is the plan to roll this out? Are we doing a trial run of sorts?

@mrgerardorodriguez I think anybody can claim it. Maybe a sensible division of labor would be:

  1. the helpers themselves
  2. the associated tests

Separate PRs could be issued for each (or for even smaller bites). Also, we can assess the linter configuration itself during this process. There may be rules that need changing. I took the values from what's being used on another project, since it happened to have the most rules defined. These rules were written by a single person though, so I expect some differing opinions.

Regarding new syntax specifically...

The linter configuration will force some of it, but other things (like when to use arrow functions) will be open-ended. Also, we're currently limited to what's supported by Node 4.0, so that will rule out the application of some features (like default params and destructuring). You'll realize what will or won't work when you attempt to run the tests, since Node will fail.

When we're all running a newer version of Node that supports more of the syntax/features, we can do another round of refactoring that includes the fun stuff.

tylersticka commented 8 years ago

FWIW, I'd consider this a lower priority than any of our Drizzle stuff.