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 accessing window and navigator global objects, fix Url import #709

Merged
merged 7 commits into from
May 28, 2024

Conversation

blazejkustra
Copy link
Contributor

@blazejkustra blazejkustra commented May 28, 2024

This is a follow up PR to the setup of TS in this repo. While testing in the E/App here I noticed two issues:

  1. Url export is wrong (I tested it before but missed that Url was undefined 🤦)
  2. Importing from a main file index.ts causes other exported files to run (for example BrowserDetect.jsx). This causes problems because code in expensify-common assumes that window and navigator global objects exist, which isn't always true.

Fixed Issues

$ Expensify/App#42245

@blazejkustra blazejkustra changed the title Fix/global objects Fix accessing window and navigator, fix Url import May 28, 2024
@blazejkustra blazejkustra changed the title Fix accessing window and navigator, fix Url import Fix accessing window and navigator global objects, fix Url import May 28, 2024
@@ -483,10 +483,10 @@ export default class ExpensiMark {
Log.alert('[ExpensiMark] Missing account name', {accountID: g1});
return '@Hidden';
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a prettier diff on main

Copy link
Contributor

@Skalakid Skalakid left a comment

Choose a reason for hiding this comment

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

LGTM 💥

@blazejkustra blazejkustra marked this pull request as ready for review May 28, 2024 14:12
@blazejkustra blazejkustra requested a review from a team as a code owner May 28, 2024 14:12
@melvin-bot melvin-bot bot requested review from thienlnam and removed request for a team May 28, 2024 14:13
@blazejkustra blazejkustra marked this pull request as draft May 28, 2024 14:31
@blazejkustra blazejkustra marked this pull request as ready for review May 28, 2024 15:18
@blazejkustra
Copy link
Contributor Author

Not sure who's going to review and merge this one as @roryabraham has context on it, but @thienlnam was assigned. Either way the PR is ready, thank you!

@roryabraham roryabraham merged commit 824327a into Expensify:main May 28, 2024
6 checks passed
Copy link

🚀Published to npm in v2.0.2

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.

3 participants