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

fix: normalize paths in get-options-overrides #331

Merged

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented May 26, 2022

Summary

get-options-overrides.ts had some paths that were not normalized, resulting in mixed separators on Windows in certain cases.

Details

  • the outDir was not normalized after the /placeholder part was added to cacheRoot

    • cacheRoot could have \ directory separators on it on Windows, which caused some tests to fail on Windows before
    • tests have been normalized now too
  • expandIncludeWithDirs used path.join without normalizing after

    • path.join uses the OS's native separators (posix.join would do POSIX separators only), so when the paths were already normalized and then path.joined, this would cause mixed separators on Windows
    • this fixes the current CI failure on Windows in the createFilter tests (rootDirs and projectReferences, which use expandIncludeWithDirs)

Review Notes

This should just be a bugfix and not breaking, but per the linked discussions, I'm really not sure how these bugs haven't been found yet by consumers -- I'm guessing that TS does normalization on outDir (lots of redundant normalization necessary due to ambiguous inputs) and that rootDirs and projectReferences just aren't used by many Windows users.

- the `outDir` was not normalized after the `/placeholder` part was
  added to `cacheRoot`
  - `cacheRoot` could have `\` directory separators on it on Windows,
    which caused some tests to fail on Windows before
  - tests have been normalized now too

- `expandIncludeWithDirs` used `path.join` without normalizing after
  - `path.join` uses the OS's native separators (`posix.join` would do
    POSIX separators only), so when the paths were already normalized
    and then `path.join`ed, this would cause mixed separators on Windows
  - this fixes the current CI failure on Windows in the `createFilter`
    tests (`rootDirs` and `projectReferences`, which use
    `expandIncludeWithDirs`)
    - c.f. https://github.com/ezolenko/rollup-plugin-typescript2/runs/6516149780?check_suite_focus=true
@agilgur5 agilgur5 added kind: bug Something isn't working properly topic: OS separators Path normalization between OSes. Windows = '\', POSIX = '/'. Rollup resolve = native, TS = POSIX labels May 26, 2022
@agilgur5 agilgur5 requested a review from ezolenko May 26, 2022 19:48
@agilgur5
Copy link
Collaborator Author

All tests pass on Windows now! Hoping we don't have to deal with path normalization again for a while now 😅

@ezolenko ezolenko merged commit d318e9a into ezolenko:master May 30, 2022
@agilgur5 agilgur5 deleted the fix-normalize-get-options-overrides branch July 2, 2023 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working properly topic: OS separators Path normalization between OSes. Windows = '\', POSIX = '/'. Rollup resolve = native, TS = POSIX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants