fix: doclinks should not select a default platform #1882
Conversation
blakef
changed the title
packages/cli-tools/src/doclink.ts
Outdated
| if (platform) { | ||
| url.searchParams.set('platform', platform); | ||
| } |
There was a problem hiding this comment.
I don't have context on this code, but is there a way we can force relevant call sites to specify a platform where one is needed?
There was a problem hiding this comment.
I'm not sure I understand the question, but giving it a go. In some places where we take care of this. For example when the user runs npx react-native run-android, we set the platform to android.
Is that what you're asking about?
There was a problem hiding this comment.
What I mean is: Is it useful that platform is optional on line 31? Could we, for example, make it required in every call to doclink?
There was a problem hiding this comment.
I agree. There are places where platform would be helpful and is missing (e.g. runBundleInstall.ts). Can we make platform mandatory and allow null or "none" as an input when it's not necessary? I believe it should help anyone at least consider whether nulling the platform is correct choice
|
There hasn't been any activity on this pull request in the past 3 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. |
|
@blakef would you mind revisit this PR and check the suggestions? |
|
Thanks for the nudge @thymikee. I'll take a look at it. |
Previously this would default to Android, which isn't intuitive. Also fixed some links to be more specific. The new api is: link.docs(<path>, <platform>) For platforms your choices are: - ios - android - none -> drop the platform parameter if the docs don't have any platform specific relevance. - inherit -> use this if you have to `link.setPlatform` before hitting a cli-command (will assert and fail at runtime if you haven't). See the `upgrading` command for an example.
blakef
force-pushed
the
fix-doclinks-no-default-platform
branch
from
ddfc176 to
5b1af3f
Compare
|
@thymikee I think we're GTG. |
Previously this would default to Android, which isn't intuitive. Also fixed some links to be more specific.
Summary:
Before
After
Test Plan:
Built and ran the cli to
inita new project.