-
Notifications
You must be signed in to change notification settings - Fork 5
[Issue #387]: Resolved Storybook build error #388
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, some nits:
- for the PR title and for the squash commit message when you merge, can you follow git commit message conventions here: https://cbea.ms/git-commit/ (sentence case not title case, and imperative voice not past tense i.e. "Fix Storybook build error" rather than "Fixed Storybook Build Error"
- can you add more context to the PR description explaining the root cause of the issue? i want to see an explanation of the fact that __dirname is no longer available by default and perhaps include links to the stackoverflow article you found the solution in and the official node docs explaining the error
thanks! fyi nit for the future, PR titles / commit messages should use imperative voice e.g. "Resolve Storybook build error" instead of past tense "Resolved Storybook build error" |
Oh shoot, thanks for flagging that. I changed it to be sentence case and forgot about the tense. I'll remember that next time, thanks! |
## Ticket Resolves #387 ## Changes <!-- What was added, updated, or removed in this PR. --> - Updated .storybook/main.js => .storybook/main.mjs based on ECMAScript Modules instead of CommonJS - **.storybook/main.mjs:** ``` path.resolve(import.meta.dirname, "../next.config.js") ``` ## Context for reviewers <!-- Testing instructions, background context, more in-depth details of the implementation, and anything else you'd like to call out or ask reviewers. --> Originally made an update to Storybook in #388 meant to address #387. Making these changes caused issues in [platform-test-nextjs](https://github.com/navapbc/platform-test-nextjs/actions/runs/13999633004/job/39317883628). The steps taken to resolve are in this [PR](navapbc/platform-test-nextjs#98). ## Testing <!-- Provide evidence that the code works as expected. Explain what was done for testing and the results of the test plan. Include screenshots, [GIF demos](https://getkap.co/), shell commands or output to help show the changes working as expected. ProTip: you can drag and drop or paste images into this textbox. --> Making this change in platform-test-nextjs resulted in a successful app build (save for a vulnerability scan). This change also follows [Node.js documentation](https://nodejs.org/docs/latest/api/esm.html#no-__filename-or-__dirname) on what they recommend to use in place of `__dirname`.
Ticket
Resolves #387
Changes
Updated the
.storybook/main.js
in the top-level app directory.Context for reviewers
The error occurs when running
npm run storybook
from the terminal to build a dev Storybook environment locally. I added the code below at the top of the file to explicitly set a __dirname value.According to Node.js documentation,
__dirname
is unavailable in ES modules. They recommend import.meta.dirname instead to replicate this behavior.I was able to find use cases for the recommended solution in this Stack Overflow thread as well as this Medium blog article.
Testing
Storybook builds and runs locally in
http://localhost:6006
as expected when the above command is used.