Skip to content

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

Merged
carlosmiceli merged 17 commits into
Expensify:mainfrom
blazejkustra:ts/str
Jun 5, 2024
Merged

[TS migration] Migrate str to Typescript (expensify-common)#713
carlosmiceli merged 17 commits into
Expensify:mainfrom
blazejkustra:ts/str

Conversation

@blazejkustra

Copy link
Copy Markdown
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

@Skalakid Skalakid left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread lib/str.ts Outdated
Comment thread lib/str.ts Outdated
Comment thread lib/str.ts Outdated
Comment thread lib/str.ts Outdated
Comment thread lib/str.ts Outdated
Comment thread lib/str.ts Outdated
Comment thread lib/str.ts Outdated
Comment thread lib/str.ts Outdated
Comment thread lib/str.ts Outdated
blazejkustra and others added 9 commits June 4, 2024 11:35
Co-authored-by: Michał Skałka <39538890+Skalakid@users.noreply.github.com>
Co-authored-by: Michał Skałka <39538890+Skalakid@users.noreply.github.com>
Co-authored-by: Michał Skałka <39538890+Skalakid@users.noreply.github.com>
Co-authored-by: Michał Skałka <39538890+Skalakid@users.noreply.github.com>
Co-authored-by: Michał Skałka <39538890+Skalakid@users.noreply.github.com>
Co-authored-by: Michał Skałka <39538890+Skalakid@users.noreply.github.com>
Co-authored-by: Michał Skałka <39538890+Skalakid@users.noreply.github.com>
Co-authored-by: Michał Skałka <39538890+Skalakid@users.noreply.github.com>
Co-authored-by: Michał Skałka <39538890+Skalakid@users.noreply.github.com>
@blazejkustra blazejkustra requested a review from Skalakid June 4, 2024 09:37
Comment thread lib/str.ts Outdated
Comment thread lib/str.ts Outdated
Comment thread lib/str.ts Outdated
Comment thread lib/str.ts Outdated
Comment thread lib/str.ts Outdated
Comment thread lib/str.ts Outdated
Comment thread lib/str.ts
blazejkustra and others added 2 commits June 5, 2024 16:59
Co-authored-by: Viktoryia Kliushun <vikstash@gmail.com>
@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
@github-actions

github-actions Bot commented Jun 5, 2024

Copy link
Copy Markdown

🚀Published to npm in v2.0.10

@aldo-expensify

Copy link
Copy Markdown
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

blazejkustra commented Aug 26, 2024

Copy link
Copy Markdown
Contributor Author

@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

aldo-expensify commented Aug 26, 2024

Copy link
Copy Markdown
Contributor

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

blazejkustra commented Aug 26, 2024

Copy link
Copy Markdown
Contributor Author

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

aldo-expensify commented Aug 26, 2024

Copy link
Copy Markdown
Contributor

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