Skip to content

Conversation

@ianyong
Copy link
Member

@ianyong ianyong commented Aug 21, 2023

Context

The default operation of the node:path module varies based on the operating system on which a Node.js application is running. Specifically, when running on a Windows operating system, the node:path module will assume that Windows-style paths are being used.

-- https://nodejs.org/api/path.html#windows-vs-posix

While the above behaviour is useful when it comes to manipulating files on the host operating system (OS)'s filesystem, we want the preprocessor to make use of POSIX file paths because BrowserFS in the frontend is a POSIX filesystem.

Note that it is technically not wrong to make use of Windows file paths in the preprocessor, but this would introduce additional complexity into the tests (because they would need to handle file paths in both systems) as well as other parts of the preprocessor (because the Windows drive letter contains characters which are not valid JavaScript names). In fact, if js-slang were to support running multiple-file programs on the host OS in the future, this behaviour might even be necessary. However until that is the case, we should apply the YAGNI principle to keep things simple.

Changes

  • Make use of the POSIX specific implementations of the path methods regardless of OS by importing from node:path/posix

How to Test

There should be no change in behaviour on POSIX OSes. I will need someone using Windows to verify that the preprocessor tests all pass.

@ianyong ianyong requested a review from leeyi45 August 21, 2023 13:33
@coveralls
Copy link

Pull Request Test Coverage Report for Build 5926972060

  • 8 of 8 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 82.893%

Totals Coverage Status
Change from base Build 5838197500: 0.0%
Covered Lines: 10735
Relevant Lines: 12523

💛 - Coveralls

@RichDom2185 RichDom2185 self-requested a review August 21, 2023 13:56
Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

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

I was just setting up js-slang on Windows and it seems like both the master branch and this branch are failing yarn test

@leeyi45 do you know if this is expected? Generally, I use WSL when developing on my Windows machine, instead of the Windows file system directly.

@ianyong
Copy link
Member Author

ianyong commented Aug 21, 2023

I was just setting up js-slang on Windows and it seems like both the master branch and this branch are failing yarn test

@leeyi45 do you know if this is expected? Generally, I use WSL when developing on my Windows machine, instead of the Windows file system directly.

The acceptance criterion for this PR is that the preprocessor tests no longer fail. Any other tests failing is out of scope.

Also, if we intend to run js-slang on Windows, it might be good to add an OS matrix strategy to the CI pipeline so that regressions that are specific to Windows can be caught early.

Copy link
Contributor

@leeyi45 leeyi45 left a comment

Choose a reason for hiding this comment

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

LGTM. Let's keep the matrix strategy idea in mind, but since I'm the only one trying to develop on Windows at the moment its not too high a priority

@leeyi45 leeyi45 merged commit 130a030 into master Aug 27, 2023
@leeyi45 leeyi45 deleted the use-posix-paths branch August 27, 2023 00:04
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.

5 participants