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.

Sign up for GitHub

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

Jump to bottom

fix: return / when calling realpathSync with / #867

Merged
merged 6 commits into from Nov 13, 2022
Merged

fix: return / when calling realpathSync with / #867

merged 6 commits into from Nov 13, 2022

Conversation

williamstein
Copy link
Contributor

@williamstein williamstein commented Oct 26, 2022 •
edited by G-Rath

@G-Rath G-Rath mentioned this pull request Oct 26, 2022
Closed
G-Rath
G-Rath requested changes Oct 29, 2022
View reviewed changes
src/__tests__/volume/realpathSync.test.ts Outdated Show resolved Hide resolved
src/volume.ts Outdated Show resolved Hide resolved
@G-Rath G-Rath changed the title fix #863 -- realpathSync('/') returns invalid result fix: return root when calling realpathSync with root Oct 29, 2022
@G-Rath G-Rath added the bug label Oct 29, 2022
@williamstein
Copy link
Contributor Author

OK, I fixed these tests.

@G-Rath
Copy link
Collaborator

G-Rath commented Nov 3, 2022

Awesome! Have you seen this comment? otherwise this looks good to go

@williamstein
Copy link
Contributor Author

I saw the comment, but I'm also not sure what to do about it, or what all of the other MS Windows conventions are with memfs, so I don't feel qualified to address it. I may come back to it later, since Windows support is a longterm goal of my project (https://cowasm.org).

@G-Rath
Copy link
Collaborator

G-Rath commented Nov 4, 2022

I was actually referring to the original comment/suggestion about using || to avoid the local variable :)

For the windows comment, thats completely fine though in future I'd recommend putting a reaction such as a thumbs up to indicate you've seen the comment and acknowledge it without any further comment yourself

@G-Rath G-Rath changed the title fix: return root when calling realpathSync with root fix: return / when calling realpathSync with / Nov 13, 2022
@G-Rath G-Rath merged commit 8d8e8d1 into streamich:master Nov 13, 2022
streamich pushed a commit that referenced this pull request Nov 13, 2022
@semantic-release-bot
chore(release): 3.4.11 [skip ci]
ce45b54 
## [3.4.11](v3.4.10...v3.4.11) (2022-11-13)

### Bug Fixes

* return `/` when calling `realpathSync` with `/` ([#867](#867)) ([8d8e8d1](8d8e8d1)), closes [#863](#863)
@streamich
Copy link
Owner

🎉 This PR is included in version 3.4.11 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@G-Rath G-Rath G-Rath approved these changes

Assignees
No one assigned
Labels
bug released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

realpathSync('/') returns invalid result (edge case bug)
3 participants
@williamstein @G-Rath @streamich