-
Notifications
You must be signed in to change notification settings - Fork 387
Conversation
React native made the move for 0.56 but we are still using 55 and no plans for upgradig yet |
Hi @cometkim First, really appreciate your help with the refactoring and redux migration here: mattermost/mattermost-webapp#1666 You have a great approach and are taking a comprehensive look at all the possible cases through your matrix 👍 Second, we have recently discussed moving the webapp to use Babel 7. Part of this will be to determine if the ability to use native browser implementations for features and only polyfill when necessary will have significant performance improvements. Given you've started the Babel 7 upgrade for redux, wondering if you were interested in participating in the effort to move the webapp to Babel 7? If so, I'd be happy to connect you with our team when you have time for it :) |
BTW we just upgraded to RN 0.57.0 that uses babel 7 |
Sounds great! Of course I interested to. actually already has tried to upgrade webapp project to use Babel 7 once, It has failed to build. Anyway, I will resume this PR first (maybe this weekend..?). I think it wouldn't affect mobile and webapp project. Thank you for notice @enahum, @jasonblais. please let me know if you started for webapp or mobile project :) |
Thanks @cometkim! Sounds great. Given the mobile projects is now using react native v57, it contains the babel 7 upgrade. But once we've merged the Redux PR, we can work together on adding babel 7 for the webapp project. |
a733079
to
4ed2391
Compare
@jasonblais @enahum Ready to merge. |
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.
Awesome! That was quick
@cometkim We're working on moving webapp to babel 7 now as well |
How is it going? It seems you've done at mattermost/mattermost-webapp@a825d5c. |
are also need to be updated. |
@cometkim Do those need to be updated on the webapp or redux repository? |
@jasonblais only on the redux repo and each can be handled in other PRs. |
package.json
Outdated
"babel-core": "6.26.3", | ||
"babel-eslint": "8.2.3", | ||
"babel-loader": "7.1.4", | ||
"@babel/cli": "^7.1.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.
Can we set all these to be exact please
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.
@enahum yep.
@enahum Any updates? |
- Change config file to babel.config.js - Add class-properties proposal - Enable loose mode
This reverts commit 31eb0f5.
// See LICENSE.txt for license information. | ||
|
||
module.exports = { | ||
presets: [ |
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.
Can you modify this to match the webapp config? https://github.com/mattermost/mattermost-webapp/blob/master/.babelrc
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.
Just out of curiosity why it should match the webapp?
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.
@crspeller I updated it to match with webapp's.
but I think that mattemrost-redux not only for mattermost-webapp and mattermost-mobile, but also it is a reference client implementation of the API. So future, We need to support a bit more compatibility and other specs like es module.
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.
I was just asking about browser support. Which should probably be unified across our repositories. I agree that our support for this repository should be more broad.
3a7ba48
to
a31cb31
Compare
* Upgrade to babel 7 * Disable module transform in webpack build * Bump up versions of Babel deps * Update Babel 7 configuration file - Change config file to babel.config.js - Add class-properties proposal - Enable loose mode * Disable module transform by default, is it better? * Revert "Disable module transform by default, is it better?" This reverts commit 31eb0f5. * Fix mocha setup * Disable loose mode, it breaks test * Fix webpack config * Update babel dependencies * Fix devDeps with exact version notation * Fix target settings of babel/env preset to match with webapp * Set debug true if not production build * Ignore eslint on babel config
Summary
Upgrade babel to 7 beta. (many other projects are already moved e.g. React Native)
See https://babeljs.io/docs/en/next/v7-migration
useBuiltIns: "usage"
Looks just fine for this package, but need to prepare for webapp and mobile either.
Ticket Link
None
Checklist
[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]
make check-style
to check for style errors (required for all pull requests)make test
to ensure unit tests passedmake flow
to ensure type checking passedAdded or updated unit tests (required for all new features)All new/modified APIs include changes to the driversTest Information
This PR was tested on: Ubuntu Xenial