Skip to content

Nextjs: Improve support for Windows-style paths#23695

Merged
valentinpalkovic merged 3 commits into
storybookjs:nextfrom
T99:next
Oct 2, 2023
Merged

Nextjs: Improve support for Windows-style paths#23695
valentinpalkovic merged 3 commits into
storybookjs:nextfrom
T99:next

Conversation

@T99
Copy link
Copy Markdown
Contributor

@T99 T99 commented Aug 3, 2023

Added more portable path handling code to better support win32-style paths. Prior to this PR, attempting to use fonts via `next/font/local' in Storybook on a Windows machine would result in broken font resource paths.

Closes #21968

What I did

Added more portable path handling code to better support win32-style paths. As demonstrated by #21968, a bug existed with the current way that paths were being translated between next/font and Storybook. This PR utilizes more portable path handling logic and sanitizes win32-style paths to the Unix-style paths expected in standard web URIs.

How to test

  1. Open a story that uses fonts loaded via next/font on a Windows-based machine.
  2. Check the @font-face declarations that are generated into the story and ensure that the paths are correct, and that the fonts are being loaded and are available in the story.

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "build", "documentation", "maintenance", "dependencies", "other"]

@ndelangen ndelangen added windows bug ci:normal Run our default set of CI jobs (choose this for most PRs). patch:no labels Aug 7, 2023
@ndelangen ndelangen self-assigned this Aug 7, 2023
@yannbf
Copy link
Copy Markdown
Member

yannbf commented Sep 19, 2023

Hey @ndelangen would you mind checking this out whenever you have time? Thanks!

It might take some time as we are quite busy, we really appreciate your efforts and patience <3

@ndelangen
Copy link
Copy Markdown
Member

I do not have the capability to follow these manual testing steps, because I have no Windows based machine at my disposal.

I find it impossible to judge the code-change by looking at the diff alone.
I need to pass this to a maintainer that knows more about windows paths than me... @valentinpalkovic when you return, can you please look at this and review?

@valentinpalkovic
Copy link
Copy Markdown
Contributor

valentinpalkovic commented Oct 2, 2023

Hey @T99,

Well done and thank you for the provided fix!

Tested on Windows and verified, whether it's still working in non-Windows environments

@valentinpalkovic valentinpalkovic merged commit e2aa53e into storybookjs:next Oct 2, 2023
@github-actions github-actions Bot mentioned this pull request Oct 2, 2023
20 tasks
@JReinhold JReinhold changed the title Fix: Added more portable path handling code to better support win32-style paths. Nextjs: Improve support for Windows-style paths Oct 3, 2023
@yannbf yannbf mentioned this pull request Oct 17, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug ci:normal Run our default set of CI jobs (choose this for most PRs). windows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: next/fonts/local outputs with broken paths

5 participants