Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 'getPlaidLinkTokenParameters' lib to TypeScript #27527
[TS migration] Migrate 'getPlaidLinkTokenParameters' lib to TypeScript #27527
Changes from 2 commits
fffd1d9
0fe3059
341052a
4778878
b735c2a
60bc75e
967e68e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB but why are we suppressing this several times in this PR? Would it make sense to abide by this convention as part of this PR or would that require a lot of updated that are out of scope for this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cead22 This convention is more of an exception for this specific case, so I think we shouldn't apply it everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you help me understand this a little better? I don't think it's truly an exception since there's 5 occurrences in this PR, which is very small.
What are the reasons for not updating the variable names to meet the naming convention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cead22 These variables are parameters that are later sent to the API
API.read('OpenPlaidBankLogin', params, {optimisticData});
So even if we rename it here, then before sending it to the API, we will need to do something like
{redirect_uri: redirectUri}
, and it also can cause a convention warningThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, so I think the correct way to complete this issue is
If we don't do this, then we'll continue breaking our conventions forever, and continue adding exceptions, which defeats the purpose of having them.
We need to think about our code holistically and continuously update the code surrounding the code we edit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cead22 Just want to confirm, have you also updated
android_package
field to be accepted as bothandroid_package
andandroidPackage
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦 I did not, I'll do that now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cead22 Have API updates been published?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they were deployed to production yesterday!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cead22 Thank you! I've updated the PR