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: resolved ids rollup compat for win32 (#1522) #1693

Merged
merged 1 commit into from
Jan 25, 2021
Merged

feat: resolved ids rollup compat for win32 (#1522) #1693

merged 1 commit into from
Jan 25, 2021

Conversation

patak-dev
Copy link
Member

Context

This PR aligns Vite server resolved ids in Windows with the way paths are handled in Rollup. I tried to assess the impact of this change as best as I could, further review and testing may be needed.

In Rollup, if the resolveId hook defaults the resolution of absolute paths to a fully resolved path that includes the volume.

@rollup/plugin-alias internally normalizes paths by removing the volume, but this id is fed to the resolveId hook manually, and ends up being resolved so the volume is part of the final id.

The rest of the official rollup plugins, uses the createFilter(include,exclude) pattern internally that normalizes paths by switching all slashes to posix, but leaves the volume untouched.

Vite server is currently removing the volume as part of its path normalization, so the filters for rollup plugins are not compatible. The createFilter pattern is common practice in official rollup plugins, so I think it is important to try to fix this compatibility issue.

Changes

These changes only affect windows

  • normalizePath only changes slashes to posix, aligned with rollup
  • fsPathFromId keeps the volume in windows
  • tryResolveFile in vite:resolve plugin uses path.resolve in windows to ensure the volume is present

Tests

Observations

  • I also tested wmr, and it looks like it is also removing the volume so they should be experiencing the same compatibility issues
  • normalizePath from @rolluputils could be used instead of the current version. This may help avoid compatibility issues in the future.
  • In Rollup, ids are only normalized inside filters, but slashes are not normalized to posix in windows as part of the default id resolution. There remains a compat issue because of this difference with vite in @rollup/plugin-legacy (@rollup/plugin-legacy issue in windows #1521). Aligning the usage of slashes is more involved though, and I do not know how important it is. I think rollup plugins should normalize paths, and @rollup/plugin-legacy could be patched to do so.

@yyx990803
Copy link
Member

👍 Thanks for the detailed explanation!

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.

@rollup/plugin-replace issue with include/exclude options in windows
2 participants