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

Babel Support #128

Closed
wants to merge 1 commit into from
Closed

Conversation

jimjkelly
Copy link

@jimjkelly jimjkelly commented Apr 29, 2016

This PR seeks to provide a another attempt at Babel support,
in this case taking advantage of the fact that Babel out of the
box does nothing, and leaves configuration to the user.

Like #82, this implements Babel through a JavaScript processor,
but the difference is it does not implement it for .es files,
but instead replaces the default js processor with one that
always gets run through Babel. As mentioned, this does nothing
by default, which means users should notice no difference.

With the addition of a .babelrc file in the directory they run
harp from as well as installation of requisite packages, the
user can activate Babel's features - which means they can
do anything ES6 support, JSX support, the whole bag.

Some notes:

  • This seems to work with JSX despite the note in Duplicate minification of Browserified files, prevents JSX, other Browserify transforms #127.
  • added ES6 support with babel #82 sought to provide some more fancy error handling, but in trying to replicate that I found it could swallow syntax errors that prevented js from being returned at all.
  • I tried to create a test to confirm that with a .babelrc JSX would be properly transpiled, but I couldn't find a good way to do a test with a dot file short of introducing some mock library. Ideas welcome (or just an okay to add a mock library).
  • Speaking to the discussion on file extension naming, Babel these days in their examples seems to expect people to run it against .js files.

A simple example is illustrative.

$ harp init .
$ npm init  # Create a simple package.json
$ npm install babel-preset-react react react-dom

Add the following to .babelrc:

 {
    "presets": [
      "react"
    ],
}

Now, edit _layout.jade:

doctype
html
  head
    link(rel="stylesheet" href="/main.css")
  body
    != yield
    script(src="/example.js")

Edit index.jade:

h1 Welcome to Harp<span id="babel"></span>.
h3 This is yours to own. Enjoy.

Finally you can try it without Babel, this will work without the .babelrc - add an example.js with:

document.getElementById('babel').innerHTML = ' (without Babel goodness)'

Run Harp, you should see the expected page. Now try some simple React in example.js instead (ensure your .babelrc is set up):

var React = require('react')
var ReactDOM = require('react-dom')

var Goodness = React.createClass({
  render: function() {
    return <em> (with Babel goodness)</em>;
  }
});

ReactDOM.render(
    <Goodness />,
    document.getElementById('babel')
)

That should do it.

@jimjkelly
Copy link
Author

As a follow up - I've been using this on my own over the past week or so for React (JSX) and ES6 transpiling, and it has mostly worked well. The one aspect that doesn't work well is that using the new ES6 import stuff, you'd hope when that gets transpiled to a require statement that the Browserify step in Harp would handle reconciling that, but it doesn't.

In the event there ends up being interest in this - would it make sense to Browserify the results of any of the js processors after they run, instead of before?

@DawnPaladin
Copy link

I would be so happy if there was an easy way to use ES6 in Harp 😁 Hope this PR gets accepted.

@lunelson
Copy link
Contributor

lunelson commented Jul 5, 2016

@jimjkelly

EDIT -- sorry, obviously you are using it with the browserify feature if you have require statements; I'm not sure what I'm doing wrong but it chokes every single time with new JS_Parse_Error. The babel step does not seem to happen, for some reason. I will try again from a blank slate I think

@lunelson
Copy link
Contributor

lunelson commented Jul 5, 2016

Just tried applying this PR to a fresh copy of @sintaxi's master branch, it throws JS_Parse_Error on the following

console.log(`this is a template string`);

... with this in .babelrc

{
    "presets": [ "es2015" ]
}

Any ideas?

@lunelson
Copy link
Contributor

lunelson commented Jul 5, 2016

Never mind, solved it. Local package configuration problem 😅. WRT your question above concerning at which step to transform, I can't imagine why there would be a need to transform after the bundle step; but possibly, the es6 imports are not working here because it's babel-core on the raw JS file, rather than the babelify transform step with browserify? Or maybe it's because browserify is only being processed on some files (when a require() statement is detected)? In any case, given the latter, I think passing all files through babel whether browserified or not is the right choice for now, but perhaps the ultimate solution requires a better coordination of these two steps

@jimjkelly
Copy link
Author

Yeah I guess I'm curious if there's interest in this being merged from the maintainers? I'm continuing to use it daily for my work on my end, but resolving the ES6 import issue is pretty high on my list of priorities.

@lunelson
Copy link
Contributor

lunelson commented Jul 6, 2016

I wish I was more expert in how browserify and babel work together; babel on its own (based on what I can see using babeljs.io/repl) does transpile es6 import statements to require but there are so many ways an import statement can work, I'm sure it requires parsing the imported file as well, to make assignments correctly. I'm guessing this requires using the babelify transform inside the browserify process.

There is a helper function in terraform called needsBrowserify which at the moment is keying on the following Regex: /^[^#\/'"*]*(require|module|exports)\b/m—so, only looking for CommonJS syntax—I would suggest maybe forcing this to always be true, or simply removing the check, and running all JS through browserify with a babelify transform, and removing the babel transform you've put in to the js compiler. In principle, your proposal would still apply (reference .babelrc; do nothing by default); but bringing the babel transform inside the browserify step would allow (in theory) the import functionality

@jimjkelly
Copy link
Author

Yeah I too have been stumbling through this as I started using React and JSX this year, trying to learn how these tools interoperate.

And yeah I don't recall what made me shy away from using the Babelify plugin for Browserify - but actually that might work well. I don't think that makes use of the .babelrc file by default, but perhaps we could either code up that inclusion to the runtime configuration ourselves indirectory, or use some other simple method.

Because otherwise I do think the JSX/React type transforms would have to happen before the ES6 import stuff - beause for instance if you tried to import a React class I think it'll choke on the syntax.

But I'll look at maybe changing this to that method and see how it works, because that should keep it all at the same time.

@lunelson
Copy link
Contributor

lunelson commented Jul 6, 2016

Cool. Yeah I'll be interested to know how you get on. As far as I can tell with babelify's documentation, it passes its options on to babel so it might be possible to configure it the same way as you've done, i.e using .babelrc; however caveat seems to be that some options must be specified twice, once to browserify and then to the transform e.g. with extensions. Good luck!

@lunelson
Copy link
Contributor

lunelson commented Jul 7, 2016

@jimjkelly also ping me if I can help in any way

This provides Babel support as an always on for js processor, for
which the features of Babel are turned on through the user of
a .babelrc file.
@jimjkelly jimjkelly force-pushed the babel-all-the-things branch from 33837c4 to d59e976 Compare July 9, 2016 18:36
@jimjkelly
Copy link
Author

So, the approach works well for its own purposes, but it does seem to introduce some incompatibilities with the Coffee script processor, or at least it makes some tests fail. Additionally, the minification in the render call seems to make some JS tests fail.

I'd really kind of hoped to be able to at least add this in a non-breaking way to current Terraform, but it seems that isn't going to be easy.

My personal recommendation would be to remove the processors idea altogether (and coffeescript processing out of the box, I suppose) and rely on Browserify/Babel. Obviously that would be a breaking change, so couldn't go in until another major revision. The (slight) problem with that is that there doesn't currently seem to be a transform for CoffeeScript, though I don't see a reason one couldn't be made.

If there's interest in this, I may maintain a fork of Terraform/Harp that has these ideas implemented with the goal of having it go into the next major release. If there's not interest, well I may maintain a fork forever. :/ Certainly having the JSX/ES6 transforms have become integral to my workflow.

@lunelson
Copy link
Contributor

Hey @jimjkelly glad to hear you've been working on this;
there is in fact a transform for coffeescript: it was originated by the author of browserify and now maintained here

@jimjkelly
Copy link
Author

Ah, so there is! I was looking for something in Babel but I guess that works too. I integrated, and as best as I can tell it largely works, though there are several tests failing for reasons that are not immediately obvious. I need to get back to some productive work, but I'll probably push this up as a fork shortly and then keep chipping away at it.

One question that pops up - this obviates the need for the processors, but I guess keeping them around, perhaps even as empty files, would be good to maintain consistency. I know a few places in the code this is checked to build a list of supported extensions.

@lunelson
Copy link
Contributor

yeah my sense of how terraform works is that they need to be there; but could simply pass through their files unchanged, in this scenario

@jimjkelly
Copy link
Author

I'm not really working on this branch anymore and given there's no upstream interest I'll go ahead and close this.

@jimjkelly jimjkelly closed this Nov 11, 2016
@lunelson
Copy link
Contributor

Dude, it's a bummer, but I can tell you I've made some progress rolling my own compile-on-the-fly dev-server solution, and you can build something faster and more lightweight than this. It's WIP but if you're interested I'll send it to you

@jimjkelly
Copy link
Author

Yeah I'd be curious to see what you came up with. My email is on my github profile. For me I moved more towards a more traditional model of managing these various components myself, first in npm scripts and have recently been messing with webpack, trying to get some of the HMR goodness going (though it's a bit buggy).

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.

3 participants