Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Switch to babel 6 #519

Merged
merged 15 commits into from
Oct 14, 2016
Merged

Switch to babel 6 #519

merged 15 commits into from
Oct 14, 2016

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Oct 12, 2016

No description provided.

aviraldg and others added 6 commits October 2, 2016 17:27
Because the package has changed so npm can't just auto-upgrade,
so this at least tells people how to fix it when the upgrade
breaks it for everybody.
because sometimes babel can just be completely broken
@@ -0,0 +1,20 @@
var exec = require('child_process').exec;
Copy link
Member

Choose a reason for hiding this comment

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

maybe give it a shebang? #!/usr/bin/env node ?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1,4 @@
{
"presets": ["react", "es2015", "es2016", "es2017", "stage-2"],
Copy link
Member

Choose a reason for hiding this comment

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

do we really want to be using es2017 and stage-2?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, we've already started using them since we wanted class properties. Not in many places though, so could be changed.

Copy link
Member

Choose a reason for hiding this comment

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

ok that does look quite nice. I'm a bit worried about the warning at http://babeljs.io/docs/plugins/ though:

These proposals are subject to change so use with extreme caution, especially for anything pre stage-3.

Maybe we should be picking out the individual transform rather than using the stage-2 preset.

@@ -82,5 +89,8 @@
"sinon": "^1.17.3",
"source-map-loader": "^0.1.5",
"webpack": "^1.12.14"
},
"optionalDependencies": {
"babel": "6.5.2"
Copy link
Member

Choose a reason for hiding this comment

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

why is this optional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, because that was me trying to convince babel to upgrade and accidentally comitting it.

Copy link
Member Author

Choose a reason for hiding this comment

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

(done)

@@ -0,0 +1,20 @@
var exec = require('child_process').exec;
Copy link
Member

Choose a reason for hiding this comment

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

could we put this in a scripts directory?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@richvdh
Copy link
Member

richvdh commented Oct 12, 2016

lgtm modulo a few minor comments.

Out of interest, whats with the change to component-index and the use of DMRoomMap? The old var x = require('module'); no longer works?

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Oh; one more thing: upgrading babel-core will mean that the karma tests are built with babel 6 instead of babel 5. There is a comment which needs fixing in karma.conf.js.

I don't know if it will automatically pick up .babelrc. You'd think it would do, but hey, this is the javascript ecosystem, so you might have to make the presets explicit.

Was trying to force it to upgrade babel to the stub babel 6
package rather than leaving the babel 5 ones, but it's too hacky.

Also remove the outdated comment.
@dbkr
Copy link
Member Author

dbkr commented Oct 13, 2016

Yeah, https://matrix.to/#/!DdJkzRliezrwpNebLk:matrix.org/$14754047861612180rDVhF:matrix.org was where @aviraldg was explaining what changed with the component index.

as it seems to pick them up from .babelrc
InteractiveAuthEntryComponents is not a single component and
doesn't really fit into the structure: ignore it, otherwise
we crash when loading the skin.
We use instance methods (or at least, draft.js does) so we need
babel-polyfill instead.
@dbkr dbkr assigned dbkr and unassigned richvdh Oct 13, 2016
@dbkr
Copy link
Member Author

dbkr commented Oct 13, 2016

right, ptal

@dbkr dbkr removed their assignment Oct 13, 2016
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm except a couple of comments

"babel-eslint": "^6.1.0",
"babel-loader": "^5.4.0",
"babel-loader": "^6.2.5",
"babel-plugin-transform-runtime": "^6.15.0",
Copy link
Member

Choose a reason for hiding this comment

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

so is this now redundant?

@@ -0,0 +1,4 @@
{
"presets": ["react", "es2015", "es2016", "es2017", "stage-2"],
Copy link
Member

Choose a reason for hiding this comment

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

ok that does look quite nice. I'm a bit worried about the warning at http://babeljs.io/docs/plugins/ though:

These proposals are subject to change so use with extreme caution, especially for anything pre stage-3.

Maybe we should be picking out the individual transform rather than using the stage-2 preset.

@dbkr dbkr merged commit 5a9148f into develop Oct 14, 2016
This was referenced Oct 14, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants