-
-
Notifications
You must be signed in to change notification settings - Fork 827
Conversation
…into aviraldg-babelrc
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; |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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"], |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this optional?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
lgtm modulo a few minor comments. Out of interest, whats with the change to component-index and the use of DMRoomMap? The old |
There was a problem hiding this 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.
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.
right, ptal |
There was a problem hiding this 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", |
There was a problem hiding this comment.
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"], |
There was a problem hiding this comment.
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.
No description provided.