Skip to content

FIREFLY-74_upgrade_babel_and_eslint #833

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jul 18, 2019
Merged

FIREFLY-74_upgrade_babel_and_eslint #833

merged 5 commits into from
Jul 18, 2019

Conversation

deannaji
Copy link
Contributor

@deannaji deannaji commented Jul 15, 2019 •
edited
Loading

@deannaji deannaji requested review from robyww and loitly July 15, 2019 21:03
@deannaji deannaji self-assigned this Jul 15, 2019
@deannaji deannaji requested a review from ejoliet July 15, 2019 21:07
loitly
loitly approved these changes Jul 16, 2019
View reviewed changes
Copy link
Contributor

@loitly loitly left a comment

Choose a reason for hiding this comment

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

I've spot-checked firefly with the updates and it works fine. Go to go.

package.json
"@babel/polyfill": "^7.0.0",
"@babel/register": "^7.4.4",
"babel-core": "7.0.0-bridge.0",
"babel-jest": "^23.4.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it has to be declared at 2 places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think those are extra ones. I will go ahead and remove them.
Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

package.json
"@babel/plugin-proposal-json-strings": "^7.0.0",
"@babel/plugin-syntax-dynamic-import": "^7.0.0",
"@babel/plugin-syntax-import-meta": "^7.0.0",
"@babel/plugin-transform-runtime": "^7.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

The above plugins are needed now? Do you know what is requiring it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These plugins are used instead of stage-3 plugin. As far as I understood from:
https://github.com/babel/babel-upgrade
(it is mentioned at the end of that link).

Copy link
Contributor

@robyww robyww Jul 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we are using all these. I think we where using stage-3 there before for object expanding (ie.{...x, {y:1}}) when it is not part of the standard.

Let me look at each of these. I will update this comment.

We want:

  • @babel/plugin-transform-runtime

We might be using this one, if not, it is useful:

  • plugin-proposal-class-properties

I don't think we use:

  • plugin-proposal-json-strings
  • plugin-syntax-dynamic-import
  • plugin-syntax-import-meta

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I will take them out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Take them out and test, make sure nothing breaks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks!

robyww
robyww reviewed Jul 17, 2019
View reviewed changes
__jest__/jest.transform.js Outdated
module.exports = require('babel-jest').createTransformer({
presets: ["env", "react", "stage-3"],
presets: ['@babel/preset-env', '@babel/preset-react', 'stage-3'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to fixed the stage-3 thing here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@deannaji deannaji requested a review from robyww July 17, 2019 18:49
@deannaji
Copy link
Contributor Author

Some dependencies (babel/core, babel/preset-env, and babel-core bridge) has to be on devDependencies in order for jest to run.

robyww
robyww reviewed Jul 17, 2019
View reviewed changes
__jest__/jest.transform.js Outdated
module.exports = require('babel-jest').createTransformer({
presets: ["env", "react", "stage-3"],
presets: ['@babel/preset-env', '@babel/preset-react']
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not exactly what I meant. in the other webpack we now doing:

plugins: [
                    '@babel/plugin-transform-runtime',
                    '@babel/plugin-proposal-class-properties'
                ]

so I think it needs to be:

presets: ['@babel/preset-env', 
                '@babel/preset-react', 
                plugins: [
                    '@babel/plugin-transform-runtime',
                    '@babel/plugin-proposal-class-properties'
                ]]

but you should test it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks!

@deannaji deannaji requested a review from robyww July 17, 2019 20:18
@deannaji
Copy link
Contributor Author

K8s updated.

robyww
robyww approved these changes Jul 17, 2019
View reviewed changes
Copy link
Contributor

@robyww robyww left a comment

Choose a reason for hiding this comment

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

Looks good. Don't merge until you confirm @ejoliet cut the new release branch.

ejoliet
ejoliet approved these changes Jul 17, 2019
View reviewed changes
Copy link
Contributor

@ejoliet ejoliet left a comment

Choose a reason for hiding this comment

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

Release branch was cut yesterday rc-2019.2. You are good to go.

@deannaji deannaji merged commit 021177a into dev Jul 18, 2019
@deannaji deannaji deleted the FIREFLY-74_upgrade_babel_and_eslint branch July 18, 2019 19:43
@robyww robyww added the build Gradle, Webpack, package.json, Docker, babel, etc... label Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ejoliet ejoliet ejoliet approved these changes

@robyww robyww robyww approved these changes

@loitly loitly loitly approved these changes

Assignees

@deannaji deannaji

Labels
build Gradle, Webpack, package.json, Docker, babel, etc...
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@deannaji @ejoliet @robyww @loitly