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

[TS migration] Migrate str to Typescript (expensify-common) #713

Merged
merged 17 commits into from
Jun 5, 2024

Conversation

blazejkustra
Copy link
Contributor

The PR aims to migrate Str module to Typescript, it also removes almost all underscore usage from str and instead uses native JS.

Fixed Issues

$ GH_LINK

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, left some small comments connected to function descriptions, to make them have a consistent format

lib/str.ts Outdated Show resolved Hide resolved
lib/str.ts Outdated Show resolved Hide resolved
lib/str.ts Outdated Show resolved Hide resolved
lib/str.ts Outdated Show resolved Hide resolved
lib/str.ts Outdated Show resolved Hide resolved
lib/str.ts Outdated Show resolved Hide resolved
lib/str.ts Outdated Show resolved Hide resolved
lib/str.ts Outdated Show resolved Hide resolved
lib/str.ts Outdated Show resolved Hide resolved
blazejkustra and others added 9 commits June 4, 2024 11:35
Co-authored-by: Michał Skałka <[email protected]>
Co-authored-by: Michał Skałka <[email protected]>
Co-authored-by: Michał Skałka <[email protected]>
Co-authored-by: Michał Skałka <[email protected]>
Co-authored-by: Michał Skałka <[email protected]>
Co-authored-by: Michał Skałka <[email protected]>
Co-authored-by: Michał Skałka <[email protected]>
Co-authored-by: Michał Skałka <[email protected]>
Co-authored-by: Michał Skałka <[email protected]>
@blazejkustra blazejkustra requested a review from Skalakid June 4, 2024 09:37
lib/str.ts Outdated Show resolved Hide resolved
lib/str.ts Outdated Show resolved Hide resolved
lib/str.ts Outdated Show resolved Hide resolved
lib/str.ts Outdated Show resolved Hide resolved
lib/str.ts Outdated Show resolved Hide resolved
lib/str.ts Outdated Show resolved Hide resolved
lib/str.ts Outdated Show resolved Hide resolved
@blazejkustra blazejkustra requested a review from VickyStash June 5, 2024 15:05
@blazejkustra blazejkustra marked this pull request as ready for review June 5, 2024 15:54
@blazejkustra blazejkustra requested a review from a team as a code owner June 5, 2024 15:54
@melvin-bot melvin-bot bot requested review from carlosmiceli and removed request for a team June 5, 2024 15:55
@carlosmiceli carlosmiceli merged commit b9fa91c into Expensify:main Jun 5, 2024
6 checks passed
Copy link

github-actions bot commented Jun 5, 2024

🚀Published to npm in v2.0.10

@aldo-expensify
Copy link
Contributor

Hmmm, now that str.js was migrated to str.ts, Web-Expensify complains if I try to update expensify-common to the current version;

Running "browserify:web" (browserify) task
>> Error: Can't walk dependency graph: Cannot find module 'expensify-common/lib/str' from '/Users/aldocanepa/Expensidev/Web-Expensify/site/shared.js'
>>     required by /Users/aldocanepa/Expensidev/Web-Expensify/site/shared.js
Warning: Error running grunt-browserify. Use --force to continue.

I wonder if the Web-Expensify project supports ts in its dependencies or not.

@blazejkustra
Copy link
Contributor Author

blazejkustra commented Aug 26, 2024

@aldo-expensify What command do you use to update expensify-common? (I think you need to install it from npm with npm install expensify-common)

@aldo-expensify
Copy link
Contributor

aldo-expensify commented Aug 26, 2024

hmm I updated manually the expensify-common hash in package.json (it is pointing to a specific commit), then I run npm install which updates package-lock.json. Does that give a different result compared to what you proposed?

@blazejkustra
Copy link
Contributor Author

blazejkustra commented Aug 26, 2024

Does that give a different result compared to what you proposed?

Yes, this is because with TS there is a build step before the package is published to NPM. It was introduced here.

You can also try building locally to test your changes (npm run build & npm pack and copy the package contents to Expensify-Web's node_modules)

@aldo-expensify
Copy link
Contributor

aldo-expensify commented Aug 26, 2024

ahh, maybe if I had updated the imports in Web-Expensify to look like this it could have worked:

image

Currently the import looks like this:

image

I think Web-Expensify uses a pretty old/custom import system

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