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

feat(project): add flat exports for namespaced components #9008

Merged
merged 9 commits into from
Jul 23, 2021

Conversation

tay1orjones
Copy link
Member

@tay1orjones tay1orjones commented Jun 23, 2021

Closes #8901

Adds new flat exports for previously namespaced components, like PasswordInput, deprecating (with notices) the old TextInput.PasswordInput

Changelog

New

  • added flat exports for PasswordInput, ControlledPasswordInput, and FilterableMultiSelect
  • added new deprecateFieldOnObject() and deprecation notices for the old exports
  • added a migration doc too

Testing / Reviewing

  • Public API snapshot should now include these new exports at the root level in addition to retaining the old namespaced exports
  • I modified the import path of the TextInput in the TextInput story file, so if you go to that story locally and view the PasswordInput stories, you'll see the console warning pop:
    image

@netlify
Copy link

netlify bot commented Jun 23, 2021

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: b8df84e

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/60fb14e52ac8550008bbfb05

😎 Browse the preview: https://deploy-preview-9008--carbon-react-next.netlify.app

@netlify
Copy link

netlify bot commented Jun 23, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: b8df84e

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/60fb14e50c3341000753d923

😎 Browse the preview: https://deploy-preview-9008--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Jun 23, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: b8df84e

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/60fb14e51cf5a50007c3ead3

😎 Browse the preview: https://deploy-preview-9008--carbon-components-react.netlify.app

@tay1orjones tay1orjones marked this pull request as ready for review June 25, 2021 00:30
@tay1orjones tay1orjones requested review from a team as code owners June 25, 2021 00:30
@tay1orjones
Copy link
Member Author

tay1orjones commented Jun 25, 2021

This is ready for review, but I'm unsure how best to fix the tests. The public api snap updates, but the test fails because the components are calling console.warn().

I don't think we want to jest.spyOn(global.console, "warn").mockImplementation(() => {}) because that would swallow all warnings. Conditionally swallowing only console.warn for the listed components of this PR would still swallow things we'd want caught. I'm not sure what the best approach is to fix this 🤔 Any suggestions?

@tw15egan
Copy link
Collaborator

@tay1orjones is there any way to check for a specific warning message? Or a snippet from the message like "Please import and use" so that it will catch all of our deprecation warnings, but still emit the rest?

@tay1orjones
Copy link
Member Author

@tw15egan thanks for the idea! I pushed an update to do just that, it matches on the "field has been deprecated" substring.

Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

LGTM 👍 ✅

@sstrubberg sstrubberg enabled auto-merge (squash) July 14, 2021 19:24
@tay1orjones tay1orjones disabled auto-merge July 14, 2021 19:30
Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Looking great 🔥 Just left a note about the mock strategy that can help out with tests and I think after that should be good to go 🚢

packages/react/__tests__/PublicAPI-test.js Outdated Show resolved Hide resolved
packages/react/src/components/MultiSelect/index.js Outdated Show resolved Hide resolved
packages/react/src/internal/deprecateFieldOnObject.js Outdated Show resolved Hide resolved
@tay1orjones tay1orjones requested a review from joshblack July 22, 2021 20:55
@tay1orjones
Copy link
Member Author

Very much appreciate the suggestions @joshblack - all have been addressed. The mock approach feels much cleaner ✨ 👍

@kodiakhq kodiakhq bot merged commit c62b520 into carbon-design-system:main Jul 23, 2021
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.

Flatten nested imports and add deprecation notice
4 participants