Skip to content

Conversation

aligg
Copy link
Contributor

@aligg aligg commented Sep 26, 2022

Ticket

#11

Changes

  • Move from NPM to Yarn

Context for reviewers

The most interesting change is in the storybook main.js file. Magical line is {loader: 'css-loader', "options": {url: false}}, which disables url resolution. We need this b/c with the dependencies in this repo we're installing a higher version and we need to account for a breaking change from css-loader 4.0.0 around url resolution (link to changelog). This approach mirrors what next.js is doing (link) which explains why the main app was building previously but not storybook. Last piece of context, the reason this was working w/ yarn was b/c the css-loader version was for some reason a lower version.

Testing

Yay:
Screen Shot 2022-09-26 at 10 57 28 AM

Screen Shot 2022-09-26 at 10 57 19 AM

Copy link
Contributor

@sawyerh sawyerh left a comment

Choose a reason for hiding this comment

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

Thank you! Can we add an ADR for this? There wasn't one for Yarn, which made me uncertain of the original reasoning. It'd be good to document our reasoning for NPM in case a future team wants to switch to Yarn 😅

@aligg
Copy link
Contributor Author

aligg commented Sep 26, 2022

Thank you! Can we add an ADR for this? There wasn't one for Yarn, which made me uncertain of the original reasoning. It'd be good to document our reasoning for NPM in case a future team wants to switch to Yarn 😅

Added! First time writing one of these so any feedback welcome. Wasn't sure if we were supposed to autogenerate the new file or something, but I just did it manually after reading the MADR docs

Copy link
Contributor

@sawyerh sawyerh left a comment

Choose a reason for hiding this comment

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

I tested locally and successfully ran both Storybook and Next.js (including within Docker) 👍🏻

@aligg aligg merged commit 6816ffe into main Sep 26, 2022
@aligg aligg deleted the aligg/toNPM branch September 26, 2022 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants