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

Rename escape/unescape to escapeText/unescapeText #746

Merged

Conversation

tomekzaw
Copy link
Contributor

@tomekzaw tomekzaw commented Jul 8, 2024

This PR renames escape and unescape functions in Utils module to escapeText and unescapeText, accordingly.

The reason for this change is that, although deprecated, both escape and unescape are built-in JavaScript functions:

Hence, these names might be confusing for other third-party libraries that rely on this rule, e.g. Reanimated Babel plugin.

These changes are needed for ExpensiMark to work properly in worklets in react-native-live-markdown:

Fixed Issues

$ GH_LINK

Tests

  1. What unit/integration tests cover your change? What autoQA tests cover your change?
  2. What tests did you perform that validates your changed worked?

QA

  1. What does QA need to do to validate your changes?
  2. What areas to they need to test for regressions?

@tomekzaw tomekzaw requested a review from a team as a code owner July 8, 2024 13:14
@melvin-bot melvin-bot bot requested review from roryabraham and removed request for a team July 8, 2024 13:14
roryabraham
roryabraham previously approved these changes Jul 9, 2024
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

hmmm seems ok, but also I wonder if the Reanimated Babel plugin should be able to distinguish between a global function call and a function call in a module namespace.

Not really important enough that I want to block this PR, so I'm going to proceed as-is

@roryabraham
Copy link
Contributor

got some test failures though

@roryabraham roryabraham changed the title Rename escape/unescape to escapeText/unescapeText [WIP] Rename escape/unescape to escapeText/unescapeText Jul 9, 2024
@roryabraham
Copy link
Contributor

Marking as WIP until tests are ✅

@tomekzaw
Copy link
Contributor Author

I wonder if the Reanimated Babel plugin should be able to distinguish between a global function call and a function call in a module namespace.

I think this should be technically possible but I believe @tjzel can provide more details on that.

Currently we just have a list of the names of the identifiers that never get captured: https://github.com/software-mansion/react-native-reanimated/blob/77644ca17d54b20a5547e7a1be8ec279cd2b5ded/packages/react-native-reanimated/plugin/src/globals.ts#L2-L116

I'll write it down on our TODO list.

got some test failures though

My bad, I forgot to update the function names in the exports. Should be fixed now

@roryabraham roryabraham changed the title [WIP] Rename escape/unescape to escapeText/unescapeText Rename escape/unescape to escapeText/unescapeText Jul 11, 2024
@roryabraham
Copy link
Contributor

Whoops, can't merge this with unsigned commits. Please rebase and resign commits.

@tomekzaw tomekzaw force-pushed the @tomekzaw/rename-escape-unescape branch from 2fab096 to 293c238 Compare July 15, 2024 06:03
@tomekzaw
Copy link
Contributor Author

tomekzaw commented Jul 15, 2024

@roryabraham This is my first contribution to expensify-common and I forgot to enable signing commits. Sorry for inconvenience. Pushed a new signed commit with all changes.

@tjzel
Copy link

tjzel commented Jul 16, 2024

@roryabraham It seems possible to distinguish between such calls, I'll try to explore that direction in the future.

@roryabraham roryabraham merged commit 53cad96 into Expensify:main Jul 16, 2024
6 checks passed
Copy link

🚀Published to npm in v2.0.49

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.

5 participants