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

[bugfix]: support Node versions 10-14 #35

Merged
merged 2 commits into from
Feb 22, 2023

Conversation

kindoflew
Copy link
Contributor

@kindoflew kindoflew commented Feb 12, 2023

This PR fixes a bug where depngn would crash on Node versions <14. (Resolves #34)

Context

The fs/promises module was added to Node in v14. Before that, starting in v10, the Promises API was available under fs.promises (so, you'd import it like require('fs').promises). https://nodejs.org/api/fs.html#promises-api

One solution to this would have been to conditionally import the correct module using dynamic imports:

const { access } = compare(process.version, '14.0.0', '<')
    ? (await import('fs')).promises
    : await import('fs/promises');

But I didn't like the idea of littering the code with conditionals and (potentially) unnecessary async stuff. So, I opted for importing the old sync versions and wrapping them in promisify (which has the added benefit of being available starting in Node v8, so potentially more "unofficial" support?).

Changes

  • added utils directory
  • added new "promisified" versions of access and writeFile
  • moved asyncExec to utils (and removed src/queries/exec.ts)

QA

Unfortunately, I'm now on an M1 Mac which hates letting nvm install Node versions <14 for some reason, so I can't manually test this on my machine (although, I did QA with newer Node versions and ran tests and everything works).

To QA you would:

  • nvm use 12 (or any version >10 && <14)
  • run depngn
  • it should work now (it should crash without this change).

Also

Hey everybody! Hope you're all doing well!

Copy link
Contributor

@arielj arielj left a comment

Choose a reason for hiding this comment

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

looks good to me 👍

@JuanVqz
Copy link
Member

JuanVqz commented Feb 18, 2023

@kindoflew hey! everything is good so far, I hope the same is for you.

I know it's not easy to leave your package manager (I didn't want to stop using rvm for ruby stuff).

Have you heard about asdf manager?

It has an adapter for javascript, ruby and more...

I've heard they use a "pre-build" ruby to have support with M1 chips, wondering if that applies to the JS manager if you are interested in having an old version probably you can try it.

@KostiantynPopovych
Copy link
Contributor

Looks good, @kindoflew, thank you!)

@KostiantynPopovych KostiantynPopovych merged commit 280200e into upgradejs:main Feb 22, 2023
@chawes13
Copy link

chawes13 commented Mar 2, 2023

Can you please publish this when you have the chance?

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.

[Bug]: Crashes on Node v12.13.1
5 participants