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

improve regex overrides #1694

Merged
merged 13 commits into from
Sep 19, 2024
Merged

improve regex overrides #1694

merged 13 commits into from
Sep 19, 2024

Conversation

ElenaDiachenko
Copy link
Contributor

@ElenaDiachenko ElenaDiachenko commented Sep 5, 2024

Description

  • Regex overrides improvements (prevent duplication)
  • npx rnv patch reset => revert applied overrides

Related issues

Npm releases

n/a

Testing

  • yarn bootstrap-clean
  • run any platform => overrides were applied ((to make sure you can check node_modules by following the path in the summary) and .rnv/overrides folder was created in the root
  • when you run the same command again, overrides should not be applied a second time
  • you can change any override and it should change in .rnv/overrides/applied_overrides.json and be applied to node_modules.
  • run npx rnv patch reset => reverts applied overrides + deletes .rnv/overrides folder

@ElenaDiachenko ElenaDiachenko self-assigned this Sep 5, 2024
@Marius456 Marius456 added the e2e label Sep 6, 2024
@locksten locksten self-requested a review September 6, 2024 07:04
Copy link
Collaborator

@locksten locksten left a comment

Choose a reason for hiding this comment

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

Adding marker comments like this:

import * as NativeComponentRegistry from '../../NativeComponent/NativeComponentRegistry';
import {ConditionallyIgnoredEventHandlers} from '../../NativeComponent/ViewConfigIgnore';
import Platform from '../../Utilities/Platform';/*RNV*/
import NativeModules from '../../BatchedBridge/NativeModules';/*RNV*/

would break overrides in files that don't support /**/ comments, like .podspec or .properties, as well as overrides in multiline strings or macros.
But in most use cases it would be fine, so just restricting it to languages that support /**/ comments might be enough, but it would make overrides work inconsistently.

@ElenaDiachenko ElenaDiachenko marked this pull request as draft September 6, 2024 10:28
@ElenaDiachenko ElenaDiachenko marked this pull request as ready for review September 12, 2024 05:50
@pauliusguzas pauliusguzas added e2e and removed e2e labels Sep 16, 2024
@kasinskas kasinskas requested a review from locksten September 17, 2024 07:02
Copy link
Collaborator

@locksten locksten left a comment

Choose a reason for hiding this comment

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

The resetOverrides hook won't automatically run in users' projects. What exactly is it's purpose? What's the difference between it and taskPatchReset?


It seems like overrides are also applied to subdependencies now:

"@react-native-community/cli": {},
"react-native/node_modules/@react-native-community/cli": {},
"react-native-tvos/node_modules/@react-native-community/cli": {},

but weren't before (in b16b40c):

"@react-native-community/cli": {},

@RicardasN had an issue with overriding subdependencies a little while ago. Is this an intentional change for that?

Copy link
Collaborator

@pauliusguzas pauliusguzas left a comment

Choose a reason for hiding this comment

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

just merge e2e file changes from main

@ElenaDiachenko ElenaDiachenko merged commit bd09da9 into main Sep 19, 2024
2 checks passed
@ElenaDiachenko ElenaDiachenko deleted the fix/regex_overrides branch September 19, 2024 10:33
kasinskas added a commit that referenced this pull request Nov 25, 2024
This reverts commit bd09da9, reversing
changes made to 16d51b0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants