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

Props: Fix typescript unspecified default value #9873

Merged
merged 1 commit into from Feb 20, 2020
Merged

Conversation

@shilman
Copy link
Member

@shilman shilman commented Feb 16, 2020

Issue: #9827

What I did

Mapping a function that takes two arguments passes the index as the second argument. Fixed & tested!

How to test

See updated snapshots & chromatic changes

@patricklafrance
Copy link
Contributor

@patricklafrance patricklafrance commented Feb 17, 2020

@shilman
Copy link
Member Author

@shilman shilman commented Feb 17, 2020

@patricklafrance Yes, AFAICT. i'm not familiar with your code yet, so perhaps there's a better fix elsewhere. the rest of the PR is just updated (fixed) snapshots

@patricklafrance
Copy link
Contributor

@patricklafrance patricklafrance commented Feb 17, 2020

I don't understand how it could be the problem.

Furthermore if the user is not using react-docgen for TS, he doesn't go through this code. I can't find which version of SB he is running, is he on an alpha version with react-docgen?

@shilman
Copy link
Member Author

@shilman shilman commented Feb 17, 2020

@patricklafrance You don't need to know anything about the user's setup for this. The problem is repro'd multiple times in the snapshot tests, and the fix appears to be working. What am I missing?

@patricklafrance
Copy link
Contributor

@patricklafrance patricklafrance commented Feb 17, 2020

@shilman

Unless you are using react-docgen, the "typescript" codepath shouldn't be followed because it's based on the name of the type property.

In react-docgen, you have the following:

  • type -> propTypes path
  • tsType -> typescript path
  • flowType -> flow path

https://github.com/reactjs/react-docgen#result-data-structure

When using react-docgen for propTypes and react-doc-typescript you get type for both so the code always go through the "propTypes" codepath.

Overall if it fix a bug I don't mind, I am asking because I don't understand how it could fix something.

@shilman
Copy link
Member Author

@shilman shilman commented Feb 17, 2020

Does that mean that the typescript handling code is basically unused in 5.3?

@patricklafrance
Copy link
Contributor

@patricklafrance patricklafrance commented Feb 17, 2020

@shilman unless something changes after my last commit, YES.

It was going to be used once I rework the docs presets to include react-docgen-typescript-loader and provide a default config that would set typePropName to tsType.

@shilman shilman merged commit 3aa12e9 into next Feb 20, 2020
47 of 48 checks passed
47 of 48 checks passed
@github-actions
Puppeteer & A11y tests Puppeteer & A11y tests
Details
@github-actions
Automention
Details
@github-actions
CLI Fixtures
Details
@github-actions
Danger JS
Details
@github-actions
Latest CRA
Details
@netlify
Header rules No header rules processed
Details
@netlify
Pages changed 71 new files uploaded
Details
@netlify
Redirect rules No redirect rules processed
Details
@packtracker
packtracker/images No images assets found.
Details
@packtracker
packtracker/javascript 7.81 MB — ▼ 0.00%
Details
Aggregate Examples (Examples) TeamCity build finished
Details
Build (Storybook) TeamCity build finished
Details
Chromatic (Storybook) TeamCity build finished
Details
Chromatic 1 (Chromatic) TeamCity build finished
Details
Chromatic 2 (Chromatic) TeamCity build finished
Details
Chromatic 3 (Chromatic) TeamCity build finished
Details
Chromatic 4 (Chromatic) TeamCity build finished
Details
Coverage (Storybook) TeamCity build finished
Details
@github-actions
Danger All green. Nice work.
Details
DeepScan 0 new and 0 fixed issues
Details
Docs (Storybook) TeamCity build finished
Details
E2E (Storybook) TeamCity build finished
Details
Examples 1 (Examples) TeamCity build finished
Details
Examples 2 (Examples) TeamCity build finished
Details
Examples 3 (Examples) TeamCity build finished
Details
Examples 4 (Examples) TeamCity build finished
Details
Examples 5 (Examples) TeamCity build finished
Details
@netlify
Mixed content No mixed content detected
Details
Packtracker (Storybook) TeamCity build finished
Details
Smoke Tests (Storybook) TeamCity build finished
Details
Test (Storybook) TeamCity build finished
Details
Test Workflow (Storybook) TeamCity build finished
Details
ci/chromatic 9 changes accepted.
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: chromatic Your tests passed on CircleCI!
Details
ci/circleci: coverage Your tests passed on CircleCI!
Details
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: e2e Your tests passed on CircleCI!
Details
ci/circleci: examples Your tests passed on CircleCI!
Details
ci/circleci: frontpage Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: packtracker Your tests passed on CircleCI!
Details
ci/circleci: smoke-tests Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@circleci-checks
deploy Workflow: deploy
Details
@netlify
deploy/netlify Deploy preview ready!
Details
@circleci-checks
test Workflow: test
Details
@ndelangen ndelangen deleted the 9827-ts-default-values branch Feb 20, 2020
@shilman shilman added the picked label Feb 25, 2020
shilman added a commit that referenced this pull request Feb 25, 2020
Props: Fix typescript unspecified default value
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants