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

fix(typescript): Refactor carbon/react index.js to index.ts to expose new typescript typings #12787

Merged
merged 31 commits into from
Jan 6, 2023

Conversation

mbarrer
Copy link
Contributor

@mbarrer mbarrer commented Dec 5, 2022

Convert the main index.js to a ts file to expose the new typings we are adding for each component to the consumer.

Changelog

Changed

  • Renamed src/index.js to src/index.ts
  • The majority of exports in the index.ts now use export * from. This will automatically export every named export declared in the file.
  • Default export references maintained in each Component/index.js file

Testing / Reviewing

All existing component imports for the storybook should be valid and rendering. In a typescript environment, you should be able to import and reference the Checkbox component and its typings (CheckboxProps)

@mbarrer mbarrer requested a review from a team as a code owner December 5, 2022 21:35
@mbarrer mbarrer changed the title Mbarrer Refactor carbon/react index.js to index.ts to expose new typescript typings Dec 5, 2022
@netlify
Copy link

netlify bot commented Dec 5, 2022

Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit c058a9a
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/638e65056f316a0008dce3e1
😎 Deploy Preview https://deploy-preview-12787--carbon-components-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Dec 5, 2022

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit c058a9a
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/638e650525246b0009660e7d
😎 Deploy Preview https://deploy-preview-12787--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Dec 5, 2022

Deploy Preview for carbon-components-react ready!

Name Link
🔨 Latest commit c93f809
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/63b824627537400008ba1fbe
😎 Deploy Preview https://deploy-preview-12787--carbon-components-react.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Dec 5, 2022

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit c93f809
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/63b82462c7f40d0008545664
😎 Deploy Preview https://deploy-preview-12787--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@tay1orjones
Copy link
Member

@mbarrer It looks like the refactor changed the order/format of the public api snapshot and the inline snapshot of component exports. This is why the ci status check is failing.

You can update both by running yarn test -u at the root and pushing up the changes.

@mbarrer mbarrer changed the title Refactor carbon/react index.js to index.ts to expose new typescript typings fix(typescript): Refactor carbon/react index.js to index.ts to expose new typescript typings Dec 8, 2022
@mbarrer mbarrer requested a review from a team as a code owner December 8, 2022 16:08
@mbarrer
Copy link
Contributor Author

mbarrer commented Dec 8, 2022

@tay1orjones Updated, is that command generally something we should be running for every PR?

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to view the public api snapshot, and the diff is huge it won't load:

image

This file is built by looping over the keys of an object containing all the exports. My assumption is that the type of these objects has shifted some, and caused the exports to reorder, resulting in the huge diff. I think we could try to get a PR in to sort these first, then update this PR with that so the diff here might be more manageable.

It worries me to merge this without being able to verify what has changed in the public api snapshot file.

@tay1orjones
Copy link
Member

Updated, is that command generally something we should be running for every PR?

Yeah when adding types, I think the public api snapshot may change due to additions. It will likely need updated for every PR adding types for components.

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbarrer I hope you don't mind, I pushed an update here that will alphabetize exports and component api for the public api snapshot. This brought everything in line and now shows a much more small (and expected! 😅 ) diff.

Pending @jdharvey-ibm's comments, I think this looks good to go.

@mbarrer
Copy link
Contributor Author

mbarrer commented Dec 19, 2022

@jdharvey-ibm Updated with those suggestions. Overall I think this looks pretty darn clean! 😄

@kodiakhq kodiakhq bot merged commit 7dc8160 into carbon-design-system:main Jan 6, 2023
mrriteshranjan added a commit to mrriteshranjan/carbon that referenced this pull request Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants