Skip to content
This repository has been archived by the owner on Jun 27, 2022. It is now read-only.

Upgrade babel packages to v7 #241

Closed
wants to merge 2 commits into from
Closed

Conversation

harshjv
Copy link
Contributor

@harshjv harshjv commented Nov 15, 2018

Fixes #218.

In addition to this, if the target project already uses Babel 7 and has @babel/runtime as a dependency, this PR is a workaround for #211.

@harshjv harshjv mentioned this pull request Nov 15, 2018
@webmaster128
Copy link
Contributor

Why do you think this fixes #211? This does not add the runtime dependency to the packages in https://github.com/LedgerHQ/ledgerjs/tree/master/packages

@harshjv
Copy link
Contributor Author

harshjv commented Nov 15, 2018

@webmaster128 If the target project using these libraries has @babel/runtime as a dependency (probably because they are using babel 7 to transpile), this will fix it. That's why I wrote if. :)

See this https://github.com/liquality/chainabstractionlayer

@webmaster128
Copy link
Contributor

Okay, thanks for clarification. I think we can agree on If the target project already includes @babel/runtime as a dependency, this PR is a workaround for #211 but does not fix the root of #211

@harshjv
Copy link
Contributor Author

harshjv commented Nov 15, 2018

@webmaster128 yep! agreed. Let me update the description.

@webmaster128
Copy link
Contributor

As a consequence, we also have: if the target project already includes babel-runtime from Babel 6, this PR will create a missing dependency on @babel/runtime and break builds that worked before

@harshjv
Copy link
Contributor Author

harshjv commented Nov 15, 2018

@webmaster128 I think if it breaks projects currently using the old Babel, this fix can be released with a major semver release.

Also see #218 (comment)

@webmaster128
Copy link
Contributor

Well it was broken before in one way and is broken afterwards in a different way. The cleanest path is probably to fix #211 first and upgrade Babal afterwards. But with #211 fixed, target project and ledgerjs can use different Babel versions.

@harshjv
Copy link
Contributor Author

harshjv commented Nov 15, 2018

Ya, that’d be great if both projects can use different version. I’m wondering, would it not increase the size of the final build of the target project if Ledgerjs uses Babel 6 and target project uses Babel 7 or vice-versa?

@webmaster128
Copy link
Contributor

webmaster128 commented Nov 15, 2018

I’m wondering, would it not increase the size of the final build of the target project if Ledgerjs uses Babel 6 and target project uses Babel 7 or vice-versa?

Yes, since babel-runtime and @babel/runtime plus their dependencies will both be included. But I have no idea how big the impact really is.

I'd see #211 as the first step and this as a nice-to-have optimization on top of that.

"@babel/preset-flow": "^7.0.0",
"@babel/preset-react": "^7.0.0",
"@babel/preset-stage-0": "^7.0.0",
"@babel/runtime": "^7.1.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Runtime is necessary to handle babel helper aliasing in the codebase. Here: https://babeljs.io/docs/en/babel-plugin-transform-runtime#helper-aliasing

package.json Outdated
"@babel/preset-env": "^7.1.6",
"@babel/preset-flow": "^7.0.0",
"@babel/preset-react": "^7.0.0",
"@babel/preset-stage-0": "^7.0.0",
Copy link
Contributor

@chicoxyzzy chicoxyzzy Apr 22, 2019

Choose a reason for hiding this comment

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

Which stage 0 features are used in codebase?

Copy link
Contributor Author

@harshjv harshjv May 6, 2019

Choose a reason for hiding this comment

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

That's an extraneous dependency. I'll remove it.

@lacksfish
Copy link

@webmaster128 Since #211 is now closed, how should #241 be moved forward?

@webmaster128
Copy link
Contributor

@webmaster128 Since #211 is now closed, how should #241 be moved forward?

I have no idea. #332 (comment) is the best workaround I know to pull in the required runtimes manually. After all this months of all this mess with runtime dependencies of the jedgerjs I'm quite disappointed. It feels like nobody at LedgerHQ really knows what they are doing and this is a big trial and error in production software. So personally I did not look into this anymore because it somehow works for me.

@gre
Copy link
Contributor

gre commented Oct 7, 2019

If we could revive this PR and ensure it works in main depending projects like ledger-live-common or ledger-live-desktop, we would ability merge it. Afterall, this project is open source and Ledger Live team bandwidth is limited. We happily accept contributions.
Regardless if it happens via this PR or not, we have a plan to upgrade our tech stack this quarter and we hope we can make this happen too. so be sure we are going to solve this at some point.

@gre
Copy link
Contributor

gre commented Dec 17, 2019

Babel 7 has land on master branch. will shortly release new major version of the library.

@gre gre closed this Dec 17, 2019
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.

Upgrade to babel@7
5 participants