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(core): warn leading slash for output.distPath.html and default to ./ #3462

Merged

Conversation

wjw99830
Copy link
Contributor

Summary

Currently the default value of output.distPath.html is /. It's relative to output.distPath.root.
But other props(like js, css...) cannot be /, because they're relative to the root of file system.

Related Links

https://rsbuild.dev/config/output/dist-path

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

@CLAassistant
Copy link

CLAassistant commented Sep 12, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

netlify bot commented Sep 12, 2024

Deploy Preview for rsbuild ready!

Name Link
🔨 Latest commit bd22412
🔍 Latest deploy log https://app.netlify.com/sites/rsbuild/deploys/66e3c87e57e2370008ac28f9
😎 Deploy Preview https://deploy-preview-3462--rsbuild.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 76 (🟢 up 3 from production)
Accessibility: 97 (no change from production)
Best Practices: 100 (no change from production)
SEO: 92 (no change from production)
PWA: 60 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@wjw99830 wjw99830 force-pushed the feature/dist_path_leading_slash branch from 03c3183 to 8c75ea7 Compare September 12, 2024 06:27
Copy link
Member

@chenjiahan chenjiahan left a comment

Choose a reason for hiding this comment

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

I think output.distPath.html defaults to / is a mistake, it should default to ./.

By design, output.distPath.* can only be relative (except for output.distPath.root). If we allow all properties to start with /, I would worry that it might confuse some users.

@wjw99830
Copy link
Contributor Author

wjw99830 commented Sep 13, 2024

I think output.distPath.html defaults to / is a mistake, it should default to ./.

Yeah, i think so too.
But if we modify the default value to ./(means that remove removeLeadingSlash invoking for output.distPath.html), it will be a breaking change for user.

@chenjiahan
Copy link
Member

Yes, we don't want to introduce breaking changes, we can maintain compatibility with the previous behavior and print a warning if the user passes an absolute path to distPath.html

@wjw99830 wjw99830 force-pushed the feature/dist_path_leading_slash branch from 8c75ea7 to e57246c Compare September 13, 2024 03:35
Copy link
Member

Choose a reason for hiding this comment

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

The PR title can be updated~

@@ -3,7 +3,7 @@ import { join } from 'node:path';
// Paths
// loaders will be emitted to the same folder of the main bundle
export const ROOT_DIST_DIR = 'dist';
export const HTML_DIST_DIR = '/';
export const HTML_DIST_DIR = './';
Copy link
Member

Choose a reason for hiding this comment

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

The default value in documentation can be updated too

@wjw99830 wjw99830 changed the title feat(core): support leading slash for output.distPath feat(core): warn leading slash for output.distPath.html and default to './' Sep 13, 2024
@wjw99830 wjw99830 changed the title feat(core): warn leading slash for output.distPath.html and default to './' feat(core): warn leading slash for output.distPath.html and default to ./ Sep 13, 2024
Copy link
Member

@chenjiahan chenjiahan left a comment

Choose a reason for hiding this comment

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

Thanks!

@chenjiahan chenjiahan enabled auto-merge (squash) September 13, 2024 05:07
@chenjiahan chenjiahan merged commit b3967b8 into web-infra-dev:main Sep 13, 2024
9 checks passed
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.

3 participants