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

Decaffeinate #474

Closed
wants to merge 8 commits into from
Closed

Conversation

stardust66
Copy link
Contributor

Context

Summary of Changes

Two problems with this approach. First, the files that are in es6 must have the extension .es6 to be converted. Second, the converted files are automatically in strict mode. I couldn't figure out how to configure Babel when using sprockets-es6, since it uses babel through babel-source and babel-transpiler. .babelrc doesn't work, see babel/ruby-babel-transpiler#287. No configuration options are documented for sprockets-es6 and there doesn't seem like there are any based on the code. Also, sprockets-es6 is a temporary solution as Sprockets 4 natively supports es6. However, it's still in beta right now.

I can try to use webpacker as a solution but that might involve bigger changes. Although, the best solution is to switch to Sprockets 4 when a stable version gets released. What do you guys think?

Checklist

  • Tested Mobile Responsiveness
  • Added Unit Tests
  • CI Passes
  • Deploys to Heroku on test Correctly (Maintainers will handle)
  • Added Documentation (Service and Code when required)

This is the package recommended on Babel's website. However, it's
an experimental solution, because Sprockets 4 will support es6
natively. When Sprockets 4 comes out of beta we should use that
instead.
Follow tips left by decaffeinate and make style edits.
@mi-wood
Copy link
Member

mi-wood commented Mar 15, 2018

@stardust66 is there any downside to using strict mode? I'm a little confused as to why generated code uses this, since the generation itself could potentially trigger exceptions in this case. I'm not really familiar though.

@stardust66
Copy link
Contributor Author

stardust66 commented Mar 19, 2018

Strict mode: https://docs.microsoft.com/en-us/scripting/javascript/advanced/strict-mode-javascript

Strict mode throws errors when plain javascript doesn't. It restricts code for better errors and debugging. For example, in javascript you can implicitly declare variables and properties.

let test = {};
test.prop = "value";

This doesn't throw an error without strict mode. Babel uses strict mode by default because es6 modules use strict mode implicitly. I guess it's acceptable because it makes you write more proper code. If there's a way to configure sprockets-es6 I could probably turn it off but there doesn't seem to be.

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Jun 2, 2018

I am not a JavaScript coder, pretty much, so take all this with a grain of salt.

But as I see it, it seems like the choice to merge this is around whether JS coders for our project will prefer CoffeeScript or standard JavaScript going forward.

I see strict mode as being not much of a big deal, since you can still do what you need to do, it just enforces more explicit/"correct" coding... (according to strict mode's notions of "correctness.")

So if people are really hungering for standard JS code in the repo, merging this would accomplish that goal. It might not theoretically be the absolute perfect way to do it, (I wouldn't know either way) but it is a way, so it seems like it could be a good thing.

(And not that this matters a ton, but I personally find standard JS a bit more readable than CoffeeScript.)

(I found another strict mode reference, in case that's useful: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode )

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Jun 3, 2018

There is also this simpler coffescript to js converter: http://js2.coffee/
(aka https://github.com/js2coffee/js2coffee)

Not 100% sure if that is producing non es6 code (I can't tell es6 from non-es6 JavaScript), but I think so.

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.

None yet

3 participants