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

chore(rename): renames all -test files to .test #6266

Closed

Conversation

tw15egan
Copy link
Collaborator

@tw15egan tw15egan commented Jun 12, 2020

Closes #2535

Updates all -test.js files to and .test.js so that the glob successfully matches and does not include these files in our build. We also now get the custom icons for test files 🎉

Screen Shot 2020-06-12 at 1 51 05 PM

Changelog

Changed

  • Lots of filenames

Testing / Reviewing

Make sure all tests still run, and the storybook environment is the same.

Run yarn build in the react package, and ensure no test files are included in the es or lib output folders

@tw15egan tw15egan requested a review from a team as a code owner June 12, 2020 17:42
@ghost ghost requested review from emyarod and joshblack June 12, 2020 17:42
@netlify
Copy link

netlify bot commented Jun 12, 2020

Deploy preview for carbon-elements ready!

Built with commit 0c21e03

https://deploy-preview-6266--carbon-elements.netlify.app

@joshblack
Copy link
Contributor

@tw15egan this might impact some of the teams pulling in our stories potentially, just as a heads up.

cc @jendowns @tay1orjones do you all still bring in stories for your storybooks or no? 👀

@joshblack
Copy link
Contributor

Do we know why the glob wasn't working previously btw? Story changes seem great, test files would be great to keep the test file strategy consistent between standalone and ones in __tests__ if we could (*-test pattern)

@netlify
Copy link

netlify bot commented Jun 12, 2020

Deploy preview for carbon-elements ready!

Built with commit 10e5616

https://deploy-preview-6266--carbon-elements.netlify.app

@tw15egan
Copy link
Collaborator Author

👍 I threw this up to start a discussion around this, because definitely not sure how far-reaching this change can be

Not sure how much work would need to be put into exporting the stories, but can definitely look into that first if this will break people
#2570

@netlify
Copy link

netlify bot commented Jun 12, 2020

Deploy preview for carbon-components-react ready!

Built with commit 0c21e03

https://deploy-preview-6266--carbon-components-react.netlify.app

@tw15egan
Copy link
Collaborator Author

@joshblack after doing some research into why the globs weren't working it seems like babel doesn't utilize the glob package directly, so *-story was trying to match to files literally named *-story

@netlify
Copy link

netlify bot commented Jun 12, 2020

Deploy preview for carbon-components-react ready!

Built with commit 81e447f

https://deploy-preview-6266--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Jun 12, 2020

Deploy preview for carbon-components-react ready!

Built with commit 10e5616

https://deploy-preview-6266--carbon-components-react.netlify.app

@tay1orjones
Copy link
Member

@joshblack @tw15egan Yeah, we import stories from your build and include them in our storybook. We have them placed in the 'others' section and renamed it to 'Carbon' via a pseudo element overlay.

This would break that for us - we'd probably be looking at copy/pasting stories from your codebase and then manually keeping them up to date when we update to new releases. We could potentially script that, but a more formal solution for story reuse would be nice.

@tw15egan
Copy link
Collaborator Author

@tay1orjones yeah definitely don't want to break it for y'all. I haven't dug into much regarding Component Story Format, but if we could add the knobs as parameters then that seems like the next step before we go any further with these renames.

@tw15egan
Copy link
Collaborator Author

@tay1orjones I threw up a PR that is switching our Accordion to the new storybook format. Is this all that would be needed (for each component)?

#6284

@tw15egan tw15egan force-pushed the change-filenames branch 2 times, most recently from 9403546 to 5b2bed6 Compare June 24, 2020 19:26
@tw15egan tw15egan changed the title chore(rename): renames all -story,-test to .story,.test chore(rename): renames all -test to .test Jun 24, 2020
@tw15egan
Copy link
Collaborator Author

@tay1orjones @joshblack I've updated this PR to just included a change to the test file names. I have a PR up exploring exporting storybook stories in the CSF format (just Accordion for now) if y'all want to take a look. I'm not sure exactly how you are importing them now vs what you would need to do differently with the proposed format but would love to get some eyes on it.

@tw15egan tw15egan changed the title chore(rename): renames all -test to .test chore(rename): renames all -test files to .test Jun 24, 2020
@tay1orjones
Copy link
Member

@tw15egan I think that would work for us. I posted a comment on the other PR with some thoughts/questions around what it specifically might look like.

Currently we re-export Accordion in our library with no modifications. So we directly reuse your story file by having an Accordion.story.jsx that contains

export {
  default as AccordionStory,
} from 'carbon-components-react/lib/components/Accordion/Accordion-story';

@joshblack
Copy link
Contributor

@tw15egan would we want to add **/*-test.js to our .npmignore file? It might be a good short-term fix instead of needing to rename files.

@tw15egan
Copy link
Collaborator Author

@joshblack if we are concerned with unintended problems with the name change we can go that route

@joshblack
Copy link
Contributor

@tw15egan just was to minimize the number of changes that needed to happen for the issue. If we want to start communicating that test files should be written with *.test.js we could definitely make that change, just was offering something else that seemed like fewer changes in the PR and for the team.

@tw15egan tw15egan closed this Jun 25, 2020
@tw15egan tw15egan deleted the change-filenames branch April 28, 2021 18:17
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.

Babel is failing to ignore stories and tests during build
3 participants