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

Major Patch: ES6-Babel-6-Full-Dependencies-Upgrade #111

Closed
wants to merge 2 commits into from
Closed

Major Patch: ES6-Babel-6-Full-Dependencies-Upgrade #111

wants to merge 2 commits into from

Conversation

bartekus
Copy link
Contributor

-CoffeeScript has been replaced with ES6
-Babel 5 has been upgraded to 6 along with structural changes necessary
-All Dependencies but hapi have been updated to most recent versions
-lint has been modified to utilize babel
-unnecessary modules have been removed
-modified package.json enabling 'npm start' to execute "./scripts/prepublish.sh && npm list"
-linting + deployment operational

Currently the default starter needs to be modified to be operational:
-Adding dev-dependencies: babel-preset-es2015 & babel-preset-react
-Modify .babelrc -> { "presets": ["es2015", "react"] }
-Modify _template.jsx :

module.exports = React.createClass({
    constructor(props){
        super(props);
        this.state = {};
    },

  render() {...

Work-In-Progress:
-Error when trying to access http://0.0.0.0:8000/ after gatsby develop:

{
statusCode: 500,
error: "Internal Server Error",
message: "An internal server error occurred"
}

…en upgraded to 6 along with structural changes necessary -All Dependencies but hapi have been updated to most recent versions -lint has been modified to utilize babel -unnecessary modules have been removed -linting + deployment operational
@knowbody
Copy link

this looks awesome! great work @bartekus 🎉

just please don't add .es6, it's just javascript - so .js.

@KyleAMathews
Copy link
Contributor

This is great! Thanks for taking this on!

A few quick notes.

Please use .js for both normal js and jsx code. Also I noticed you removed
a few eslint rules. Please restore those. On the bin/ scripts. They either
need to remain coffeescript or use strict es5 as Babel code doesn't work in
globally installed modules (hence the recent release which now uses
compiled code). Or I suppose we could compile those modules on publish.
On Sun, Dec 27, 2015 at 12:57 PM Mateusz Zatorski notifications@github.com
wrote:

this looks awesome! great work @bartekus https://github.com/Bartekus [image:
🎉]

just please don't add .es6, it's just javascript - so .js
I'm even not a big fan of .jsx, but won't be that picky.


Reply to this email directly or view it on GitHub
#111 (comment).

@KyleAMathews
Copy link
Contributor

Oh also on ESLint, please restore the Airbnb eslint config as the default.
It has rules for react/es6 that I'm happy with and widely used.
On Sun, Dec 27, 2015 at 1:13 PM Kyle Mathews mathews.kyle@gmail.com wrote:

This is great! Thanks for taking this on!

A few quick notes.

Please use .js for both normal js and jsx code. Also I noticed you removed
a few eslint rules. Please restore those. On the bin/ scripts. They either
need to remain coffeescript or use strict es5 as Babel code doesn't work in
globally installed modules (hence the recent release which now uses
compiled code). Or I suppose we could compile those modules on publish.
On Sun, Dec 27, 2015 at 12:57 PM Mateusz Zatorski <
notifications@github.com> wrote:

this looks awesome! great work @bartekus https://github.com/Bartekus [image:
🎉]

just please don't add .es6, it's just javascript - so .js
I'm even not a big fan of .jsx, but won't be that picky.


Reply to this email directly or view it on GitHub
#111 (comment).

@KyleAMathews
Copy link
Contributor

Oh I guess things were already jsx :-) I've stopped doing that in my personal projects so forgot that's what we're doing here.

@knowbody
Copy link

also I would use react-transform-hmr as react-hot-loader is deprecated now. @bartekus if you need help with that upgrade I'm happy to help you with that

@KyleAMathews
Copy link
Contributor

Also perhaps leave the react router upgrade for a separate PR. There's a number of API changes to the router that will require the algorithm in create-routes to change. Let's get the coffeescript conversion done and then work on upgrades.

@KyleAMathews
Copy link
Contributor

👍🏻 to transform-hmr upgrade. Also add in redbox. But let's do these in stages. Finish Babel upgrade/conversion then add new features.

@knowbody
Copy link

@KyleAMathews it will probably be good to create a new issue with the upgrade roadmap

@knowbody
Copy link

Note: throwing this as an example, something to look at, while upgrading

for the react transform and redbox you can have a look at the https://github.com/FormidableLabs/spectacle repo, we've done the upgrade there, and here are the changes required for the Babel6 upgrade https://github.com/FormidableLabs/spectacle/pull/131/files

@KyleAMathews
Copy link
Contributor

#32 is sufficient I think for the initial upgrade. Once a PR for that lands, we can tackle advanced features RR 1.x+ enables like code splitting #10

@KyleAMathews
Copy link
Contributor

Or wait, which upgrade are we talking about now @knowbody? :-) Babel now?

@knowbody
Copy link

also I think React Router 2.0 is coming out soon 💃

@knowbody
Copy link

@KyleAMathews the spectacle example I've just sent is RE: react-transform upgrade and Babel 6

@KyleAMathews
Copy link
Contributor

👍🏻

@knowbody
Copy link

@KyleAMathews remix-run/react-router#2646

I haven't been around the router for a while but seems pretty close :)

@KyleAMathews
Copy link
Contributor

I think the upgrade to 2.x will be much easier than to 1.x so no need to delay upgrading to 1.x even if 2.x is close.

@timdorr
Copy link

timdorr commented Dec 27, 2015

Yes, 2.0 should be soon. And the upgrade should be more incremental than 1.0. It's mostly the history singletons and location descriptors, but should be backwards compatible until the next release.

@bartekus
Copy link
Contributor Author

My apologies gentlemen, I had to step out for a second...

Anyway, I'm ok with doing incremental patches vs this blob, so breaking it all up into a more compartmental aspects is all fine by me

As to the .es6 format, since it was replacing .coffee it was a major change and I wanted to bring attention to that. However I do agree with uniform standard of .js across the project for clarity and ease of understanding.

@KyleAMathews
Copy link
Contributor

👍🏻

@bartekus
Copy link
Contributor Author

Changing extensions along with lint does induce a little bit of errors to fix so it will take some time while I'm sorting that out.

…s will need to be rewritten for this to be on] -all jsx/es6 extension are now js -general cleanup as per lint requrement -react-router has been rolled back to it's previous version -lint with 3 warnings
@bartekus
Copy link
Contributor Author

So I've modified few things:

  • .eslintrc restored + modified with no-loop-func disabled [ glob-pages will need some tlc]
  • all jsx/es6 extension are now js -
  • general cleanup as per lint requirement
  • react-router has been rolled back to it's previous version
  • lint with 3 warnings

@bartekus
Copy link
Contributor Author

I'm now looking at resolving the error when trying to access http://0.0.0.0:8000/ after gatsby develop and once I'm done with that the babel-6 upgrade will be opperational and I'll be moving to react-transform-hmr upgrade as per our discussion.

@@ -1,4 +1,6 @@
import React, {PropTypes} from 'react'
'use strict'
Copy link
Contributor

Choose a reason for hiding this comment

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

why all the 'use strict'? My understanding is Babel adds those automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are most likely correct and being that I wasn't certain I decided to enforce it across the whole project. I'll reverse this change however given that it doesn't make much sense now.

@KyleAMathews
Copy link
Contributor

@bartekus plan sounds great!

Also I think this is what you meant but to be sure, please make the hot-module transform upgrade a separate PR to make it simpler to review and merge this PR.

Thanks for your effort!

"no-loop-func": 0,
"quotes": [2, "single"],
"strict": [2, "never"],
"react/jsx-uses-react": 2,
Copy link
Contributor

Choose a reason for hiding this comment

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

These are redundant as they're already set in the airbnb eslint config https://github.com/airbnb/javascript/blob/fbf81eba3a02ef2dc4c92b0d98f93a4bad99869d/packages/eslint-config-airbnb/rules/react.js#L70

Only add rules here if we need to override eslint defaults or rules we inherit from airbnb.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, I'll remove
"quotes": [2, "single"],
"strict": [2, "never"],
"react/jsx-uses-react": 2,
However, I'll keep "no-loop-func": 0, until glob-pages.js is modified to support this requirement.

@mattmo
Copy link

mattmo commented Feb 19, 2016

What's the status of this PR? Would be cool to see it landed 👍

@timdorr
Copy link

timdorr commented Feb 19, 2016

Looks like @KyleAMathews has been doing bits of it himself. Namely, the switch from CS to JS.

@KyleAMathews
Copy link
Contributor

Yeah WIP but should have upgrades done plus new release done by this
weekend.
On Fri, Feb 19, 2016 at 2:59 PM Tim Dorr notifications@github.com wrote:

Looks like @KyleAMathews https://github.com/KyleAMathews has been doing
bits of it himself. Namely, the switch from CS to JS.


Reply to this email directly or view it on GitHub
#111 (comment).

@KyleAMathews
Copy link
Contributor

Closing this as the work is mostly done except for the Babel 6 upgrade. I worked on that for a while but couldn't get around a weird source-map bug. I file an issue next week sometime w/ steps for reproducing. Thanks again @bartekus for all your work here!

ChristopherBiscardi pushed a commit to ChristopherBiscardi/gatsby that referenced this pull request Jun 27, 2019
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.

5 participants