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: isPromiseFs out-of-spec usage of readFile #1857

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: isPromiseFs out-of-spec usage of readFile #1857

wants to merge 3 commits into from

Conversation

mdekstrand
Copy link

@mdekstrand mdekstrand commented Jan 22, 2024 •
edited

This works around the odd Deno behavior described in #1851 by explicitly passing undefined to readFile, so the promisify wrapper works properly.

This does seem to be a Deno bug (denoland/deno#21795), but it's also an emergent behavior of isomorphic-git calling readFile out-of-spec + promisfy's inability to identify that because it is unaware of intended method signatures (resulting in a place where IMO JavaScript's correct error behavior is under-specified). This workaround gets isPromsseFs working on Deno, and that + manually adding Buffer to globalThis is enough to get isomorphic-git working on Deno for my use case.

Full Deno support (including running the tests) would be a much larger effort. Closes #1851.

I'm fixing a bug or typo

  • if this is your first time contributing, run npm run add-contributor and follow the prompts to add yourself to the README
  • squash merge the PR with commit message "fix: [Description of fix]"

@mdekstrand
Copy link
Author

Test failures are a couple of Chrome pack tests that I don't think this code touched.

@jcubic
Copy link
Contributor

jcubic commented Jan 22, 2024

It fails for Android, the the code touched is always executed so the error came from that code.
Can you try changing it to an empty string?

@mdekstrand
Copy link
Author

Changing to '' those two tests still fail, and now a 3rd does as well. I will revert to the undefined version when I get home.

The failing assertion in both versions is that the pack name does not match the expected pack name.

@jcubic
Copy link
Contributor

jcubic commented Jan 22, 2024

The last PR was this one #1853 and the test was passing.

I will try to create a trivial PR to test if the tests pass.

@jcubic
Copy link
Contributor

jcubic commented Jan 22, 2024

The tests are broken, I have no idea why. Here is a README change #1858 that is also failing.

I will merge this PR when I fix the tests, never happen before. I hope I will be able to figure out why the tests are failing.

@jcubic
Copy link
Contributor

jcubic commented Feb 15, 2024

The tests got fixed, please rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

isPromiseFs fails on Deno
2 participants
@mdekstrand @jcubic