Skip to content
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.

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

fix(babel-preset-mc-app): to add loose private methods #2203

Merged
merged 7 commits into from
May 11, 2021
Merged

Conversation

tdeekens
Copy link
Contributor

Summary

We're receiving a lot of warnings internally when building with the recent babel update with about 40k lines of this on CI:

CleanShot 2021-05-11 at 09 42 25@2x

Though the "loose" option was set to "false" in your @babel/preset-env config, it will not be used for @babel/plugin-proposal-private-methods since the "loose" mode option was set to "true" for @babel/plugin-proposal-class-properties.
The "loose" option must be the same for @babel/plugin-proposal-class-properties, @babel/plugin-proposal-private-methods and @babel/plugin-proposal-private-property-in-object (when they are enabled): you can silence this warning by explicitly adding

@vercel
Copy link

vercel bot commented May 11, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/commercetools/merchant-center-application-kit/4PidqBRUCW2iE3ZQdk1txUPi4UdH
✅ Preview: https://merchant-center-application-kit-git-td-add-loose-commercetools.vercel.app

@changeset-bot
Copy link

changeset-bot bot commented May 11, 2021

🦋 Changeset detected

Latest commit: a440704

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@commercetools-frontend/babel-preset-mc-app Patch
@commercetools-frontend/jest-preset-mc-app Patch
@commercetools-frontend/mc-scripts Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@tdeekens tdeekens requested a review from emmenko May 11, 2021 07:46
@emmenko
Copy link
Member

emmenko commented May 11, 2021

Thanks but unfortunately that does not fully work.

Last time I tried to fix that, Cypress has problems: https://github.com/commercetools/merchant-center-application-kit/runs/2509665796

I haven't looked too much into it but apparently something else needs to be fixed/changed as well.
If you have the time, feel free to look into it.

@vercel vercel bot temporarily deployed to Preview May 11, 2021 07:52 Inactive
@tdeekens
Copy link
Contributor Author

tdeekens commented May 11, 2021

For others and me to document:

We use loose on class properties as the otherwise used defineProperty comes with a performance penalty. Currently create-react-app doesn't have and special configuration for private methods.

When true, private methods will be assigned directly on its parent via Object.defineProperty rather than a WeakSet. This results in improved performance and debugging (normal property access vs .get()) at the expense of potentially leaking "privates" via things like Object.getOwnPropertyNames.

So I don't see why Cypress chokes on this as both syntaxes should work. Will investigate further.

Reference: https://babeljs.io/docs/en/babel-plugin-proposal-private-methods

@vercel vercel bot temporarily deployed to Preview May 11, 2021 08:06 Inactive
@vercel vercel bot temporarily deployed to Preview May 11, 2021 08:18 Inactive
@vercel vercel bot temporarily deployed to Preview May 11, 2021 08:42 Inactive
babel.config.js Outdated Show resolved Hide resolved
Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

Good idea!

babel.config.js Outdated Show resolved Hide resolved
.changeset/tiny-kids-wait.md Outdated Show resolved Hide resolved
packages/babel-preset-mc-app/index.js Outdated Show resolved Hide resolved
packages/babel-preset-mc-app/index.js Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview May 11, 2021 09:59 Inactive
@emmenko emmenko merged commit 99ea52d into main May 11, 2021
@emmenko emmenko deleted the td/add-loose branch May 11, 2021 10:25
This was referenced May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants