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

[legacy-framework] fix(server): force forward slashes in absolute imports #393

Merged
merged 5 commits into from
May 3, 2020

Conversation

wKovacs64
Copy link
Collaborator

Type: bug fix

Closes: blitz-js/legacy-framework#495

What are the changes and their implications? ⚙️

Sindre Sorhus to the rescue again. Slashified the result of path.join (it was trying to be helpful, but it wasn't in this particular case) to enforce forward slashes in absolute paths. A hack? Maybe. I don't know anymore.

Checklist

  • Tests added for changes
  • Any added terminal logging uses packages/server/src/log.ts

Breaking change: no

At least, I sure hope not.

Other information

CrossPlatformFileAccess50

@wKovacs64 wKovacs64 requested review from ryardley and flybayer May 1, 2020 18:40
@wKovacs64 wKovacs64 marked this pull request as draft May 1, 2020 19:01
@wKovacs64
Copy link
Collaborator Author

Still working on this, avert your eyes.

@simondebbarma
Copy link
Collaborator

@wKovacs64 Sure, I'm just organizing all Issues and PRs. Let me know if I assigned the wrong tags.

@wKovacs64
Copy link
Collaborator Author

@simonpeterdebbarma Oh you're good, that wasn't directed at you - more of a general warning for anyone who stumbled along (although I guess that should be obvious from being in draft). 🙂

@wKovacs64 wKovacs64 marked this pull request as ready for review May 1, 2020 21:33
@wKovacs64
Copy link
Collaborator Author

OK, I think this is ready.

The testing situation sucks because the implementation relies on Node's path module which returns different values based on the operating system on which the tests are running at the time. To deal with this, I added a platform check in the tests and pass the platform-specific paths based on that result. This is kind of bad because now there are 2 code paths (Windows and not-Windows) in the test suite for this module, where one executes on Windows and the other on macOS and Linux. That said, they will both be run once we have Windows as part of our OS matrix in CI (PR ready to go), so it should be clear when a change breaks one platform but not the other. An alternative approach might be to spy on path functions and mock their implementations per test but that felt worse. Open to other suggestions!

@wKovacs64 wKovacs64 force-pushed the justin/fix-relative branch from 76bddb1 to 82e9bcb Compare May 2, 2020 07:16
@ryardley ryardley merged commit 386a7fd into canary May 3, 2020
@ryardley ryardley deleted the justin/fix-relative branch May 3, 2020 07:56
@itsdillon itsdillon changed the title fix(server): force forward slashes in absolute imports [legacy-framework] fix(server): force forward slashes in absolute imports Jul 7, 2022
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.

Building store example fails on Windows
3 participants