Skip to content

Make relative effectful - #25

Open
m-rinaldi wants to merge 1 commit into
purescript-node:masterfrom
m-rinaldi:relative-is-impure
Open

m-rinaldi wants to merge 1 commit into
purescript-node:masterfrom
m-rinaldi:relative-is-impure

Conversation

@m-rinaldi

Copy link
Copy Markdown

Description of the change

relative is typed as a pure function, but Node's path.relative resolves both of its arguments against process.cwd() before computing the relative walk. Whenever either argument is a relative path, the result therefore depends on the working directory at the time of the call:

cwd=/           relative "a" "../b"  ->  "../b"
cwd=/usr/local  relative "a" "../b"  ->  "../../b"

This is the same reasoning that moved resolve into Effect in #14 (see also #4 and the discussion in #20). path.relative calls path.resolve internally and inherits the dependence, but its signature was never updated.

This PR changes relative to FilePath -> FilePath -> Effect FilePath, with the FFI written the same way as resolve. The doc comment now mentions the working-directory dependence, and the existing test is adjusted to bind the result.

This is a breaking change and will need a major version bump.


Checklist:

  • Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable): the existing relative test was updated for the new type; no new test was added, matching resolve is impure #14.

@garyb garyb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Absolutely, good catch! Surprised it's taken anyone this long to notice 😄

@m-rinaldi

Copy link
Copy Markdown
Author

Thanks! Felt like a déjà vu of #14 🔁

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.

2 participants