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 inlinePlugin replacing __DEV__ in invalid Babel AST locations #1195

Closed

Conversation

kitten
Copy link
Contributor

@kitten kitten commented Jan 19, 2024

Summary

We've encountered this as part of a report on the expo repo: expo/expo#25965

There, an issue is caused by a dependency using an export alias named __DEV__, which the inlinePlugin is trying to replace:

export { DEV as __DEV__ };

This only occurs in certain cases, since usually, as part of the iOS/Android builds, we would be applying the inlinePlugin only after export statements, where this issue may occur, have been transpiled away.

However, looking at this issue, I found more cases that would fail with the current implementation of the inlinePlugin. This is part of what I added in tests but not an exhaustive list:

  • Shorthand object methods ({ __DEV__() {} })
  • Labelled statements (__DEV__: {};)
  • Class methods (class { __DEV__() {} })
  • Optional property member access (x?.__DEV__)

All of these issues can be addressed by letting this replacement use ReferencedIdentifier as a visitor, rather than just Identifier.

Test plan

  • Tests have been added to inline-plugin-test.js to reflect the mentioned cases.

@facebook-github-bot
Copy link
Contributor

Hi @kitten!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@kitten kitten force-pushed the @kitten/fix/inline-dev-identifier branch from d7871fe to 7857c4d Compare January 19, 2024 13:57
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (702e1b8) 83.31% compared to head (7857c4d) 83.31%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1195      +/-   ##
==========================================
- Coverage   83.31%   83.31%   -0.01%     
==========================================
  Files         207      207              
  Lines       10633    10631       -2     
  Branches     2642     2640       -2     
==========================================
- Hits         8859     8857       -2     
  Misses       1774     1774              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Jan 19, 2024
@motiz88 motiz88 self-requested a review January 19, 2024 15:59
Copy link
Contributor

@motiz88 motiz88 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for catching this and for adding tests!

@facebook-github-bot
Copy link
Contributor

@motiz88 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@motiz88 merged this pull request in 8eeb5db.

@robhogan robhogan mentioned this pull request Jan 29, 2024
robhogan pushed a commit that referenced this pull request Jan 29, 2024
…#1195)

Summary:
We've encountered this as part of a report on the `expo` repo: expo/expo#25965

There, an issue is caused by a dependency using an export alias named `__DEV__`, which the `inlinePlugin` is trying to replace:

```js
export { DEV as __DEV__ };
```

This only occurs in certain cases, since usually, as part of the iOS/Android builds, we would be applying the `inlinePlugin` only after export statements, where this issue may occur, have been transpiled away.

However, looking at this issue, I found more cases that would fail with the current implementation of the `inlinePlugin`. This is part of what I added in tests but not an exhaustive list:
- Shorthand object methods (`{ __DEV__() {} }`)
- Labelled statements (`__DEV__: {};`)
- Class methods (`class { __DEV__() {} }`)
- Optional property member access (`x?.__DEV__`)

All of these issues can be addressed by letting this replacement use `ReferencedIdentifier` as a visitor, rather than just `Identifier`.

Pull Request resolved: #1195

Test Plan: - Tests have been added to `inline-plugin-test.js` to reflect the mentioned cases.

Reviewed By: huntie

Differential Revision: D52909519

Pulled By: motiz88

fbshipit-source-id: 37a6459bc917701fe8474c33ccae41cb484e92f0
robhogan pushed a commit that referenced this pull request Jan 30, 2024
…#1195)

Summary:
We've encountered this as part of a report on the `expo` repo: expo/expo#25965

There, an issue is caused by a dependency using an export alias named `__DEV__`, which the `inlinePlugin` is trying to replace:

```js
export { DEV as __DEV__ };
```

This only occurs in certain cases, since usually, as part of the iOS/Android builds, we would be applying the `inlinePlugin` only after export statements, where this issue may occur, have been transpiled away.

However, looking at this issue, I found more cases that would fail with the current implementation of the `inlinePlugin`. This is part of what I added in tests but not an exhaustive list:
- Shorthand object methods (`{ __DEV__() {} }`)
- Labelled statements (`__DEV__: {};`)
- Class methods (`class { __DEV__() {} }`)
- Optional property member access (`x?.__DEV__`)

All of these issues can be addressed by letting this replacement use `ReferencedIdentifier` as a visitor, rather than just `Identifier`.

Pull Request resolved: #1195

Test Plan: - Tests have been added to `inline-plugin-test.js` to reflect the mentioned cases.

Reviewed By: huntie

Differential Revision: D52909519

Pulled By: motiz88

fbshipit-source-id: 37a6459bc917701fe8474c33ccae41cb484e92f0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants