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

Migrate from CommonJS to ESM and add Windows 11 support #214

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

Conversation

HarryGwinnell
Copy link

@HarryGwinnell HarryGwinnell commented Dec 7, 2021

Apologies in advance for the long PR, hoping to trigger some discussion here so it's relevant to include the whole thing. I can break this down into smaller subsequent PRs if this change is desired.

To preface, this work came out of investigation of #213, and the fact that os-name (https://github.com/sindresorhus/os-name) has migrated to an ESM package. In order to maintain this dependency, and import the latest version which removes the dependency on wmic (which is removed in latest versions of Windows 10 and 11), that required that envinfo also switch to ESM. This should close #209 and #213

envinfo on Windows 11 build 22509 today:

Error: Command failed: wmic os get Caption
'wmic' is not recognized as an internal or external command,
operable program or batch file.


    at b (C:\Users\harry\Code\envinfo\node_modules\envinfo\dist\envinfo.js:1:95303)
    at Function.e.exports.sync (C:\Users\harry\Code\envinfo\node_modules\envinfo\dist\envinfo.js:1:97166)
    at e.exports (C:\Users\harry\Code\envinfo\node_modules\envinfo\dist\envinfo.js:1:93520)
    at e.exports (C:\Users\harry\Code\envinfo\node_modules\envinfo\dist\envinfo.js:1:92667)
    at C:\Users\harry\Code\envinfo\node_modules\envinfo\dist\envinfo.js:1:91417
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async Promise.all (index 0) {
  code: 1,
  stdout: '',
  stderr: "'wmic' is not recognized as an internal or external command,\r\n" +
    'operable program or batch file.\r\n',
  failed: true,
  signal: null,
  cmd: 'wmic os get Caption',
  timedOut: false
}

envinfo on Windows 11 build 22509 after this PR:

yarn run v1.22.11
$ node src/cli.mjs

  System:
    OS: Windows 11 10.0.22567
    CPU: (24) x64 AMD Ryzen 9 5900X 12-Core Processor
    Memory: 16.47 GB / 31.92 GB
  Binaries:
    Node: 14.18.3 - ~\AppData\Local\Temp\yarn--1646941639612-0.9770316808096613\node.CMD
    Yarn: 1.22.11 - ~\AppData\Local\Temp\yarn--1646941639612-0.9770316808096613\yarn.CMD
    npm: 8.2.0 - C:\Program Files\nodejs\npm.CMD
  Utilities:
    Git: 2.33.0. - C:\Program Files\Git\cmd\git.EXE
  IDEs:
    Android Studio: 2020.3.0.0 AI-203.7717.56.2031.7678000
    VSCode: 1.64.2 - C:\Users\harry\AppData\Local\Programs\Microsoft VS Code\bin\code.CMD
    Visual Studio: 17.1.32120.378 (Visual Studio Community 2022), 16.11.32106.194 (Visual Studio Community 2019)
  Languages:
    Java: 11.0.12 - C:\Program Files\Microsoft\jdk-11.0.12.7-hotspot\bin\javac.EXE
  Browsers:
    Edge: Spartan (44.22567.200.0), Chromium (98.0.1108.43)
    Internet Explorer: 11.00.22567.1

Done in 1.69s.

What this PR does:

  • Switch from CJS to ESM
  • Replace existing require statements with imports
  • Upgrade most dependencies to ones that properly support ESM modules (the latest versions in most cases)
  • Fix test compatibility to work with ESM
  • Switch from minimist to yargs, as minimist does not support ESM
  • Some minor housekeeping (eslint and prettier config updates to maintain status quo)

What this PR does not do:

  • Compile to executables - I haven't dug too deep on this but pkg needs some work to support ESM modules. If we want to go down the ESM route, I'll investigate further EDIT: Switched to mjs and this works now
  • Implement new eslint rules - As part of upgrades, eslint introduced a new "no-promise-executor-return" rule, and I've simply inline supressed the 4 occurrences that popped up
  • Address the two remaining wmic references in the codebase, in browser.js:159 and ides.js:49. These don't break envinfo at the moment so I've left them for followup. EDIT: Fixed

@HarryGwinnell
Copy link
Author

Made a few changes here to remove the type: module and switch to using .mjs files for those files that use ESM

@tabrindle
Copy link
Owner

This is fantastic work, thank you for contributing!

  • I'm open to leaving the last two tasks unfinished if tickets are made and you promise to follow up!
  • Outstanding questions: Does this still support very old versions of node?
  • Can we document how third party libraries will be able to use individual helper functions?
  • Are there any API changes? I'm inclined to do a major version bump with this regardless.

@HarryGwinnell
Copy link
Author

HarryGwinnell commented Dec 7, 2021

  • Happy to leave the two tasks open and loop back for them. Eslint is just a case of deciding whether we want the rule supressed or to fix these root causes. The wmic use I'll have to dig into replacing but I don't mind looking into that.
  • Unfortunately os-name has moved it's minimum version of node to ^12.20.0, and the node:os use also rules out some older 14.x versions, so that does present a breaking change.
  • I can certainly take a look at this
  • Not at the moment, this is a primarily structural upgrade just to maintain existing functionality. A major bump probably isn't a bad idea though.

I'm not sure why the rest of the checks are failing, I might have missed some changes one of the CI pipelines, will follow up shortly.

Would it be cheeky to add myself to the contributors in the Readme @tabrindle 😉

@tabrindle
Copy link
Owner

  • Awesome. Eslint is there to help us not screw things up, I dont think thats whats happening here.
  • By using webpack and bundling, can we continue using older node versions? This is a pretty important sticking point of this library - to diagnose whats going on with environments, which is a bit defeated if we cant support even recent node versions. We may have to find a way around this, as a version of node 10.x was released just 8 months ago.
  • Yay, documentation is good.
  • Agreed, major bump it is.
  • CI pipelines appear to be failing because of other tooling. We may consider building on our target dev environment, then fanning out with the built code to test it, instead of attempting to build on older versions. @gengjiawen and I have talked about this before, but it hasn't been implemented yet.
  • Go ahead and add yourself, I'd say you've earned it!

@HarryGwinnell
Copy link
Author

For compatibility, on my cursory testing, with a couple of tweaks that I'll push later, the dist package will run on Node 10.13 and above (the first 10.,x LTS build), though it doesn't currently run in 8.x. I can do some digging for 8.x support though I'm not so confident on that one

@HarryGwinnell
Copy link
Author

  • Will add the two new issues once this merges, as they're not relevant until this does
  • After a review, made a couple of tweaks and I've confirmed that the compiled version will run on anything 10.13.0 (earliest Node v10 LTS from 30-10-2018) or above. I did test on 10.0.0 but it doesn't work there. We're out of luck on Node 8 as os-name breaks Node 8, and that's the whole root of this issue.
  • Made some slight tweaks to how the module is handled, and it now perfectly matches the existing API using UMD, so no need for docs update (back compat ftw). We can look to export the helper functions as an additional export in the future, though currently we only serve the compiled UMD module, so we'd need to look at exporting both. Probably best left for a future minor release.

Otherwise I think this is as much as is going to fit into this PR. Anything else you want me to address?

@HarryGwinnell
Copy link
Author

@tabrindle Just checking if you've had a chance to look at this

@tebeco
Copy link

tebeco commented Jan 29, 2022

can confirm create-react-app crash on that error for now ( @tabrindle )

hey it's nice to see it's been working on.
thx a lot @HarryGwinnell

npx create-react-app --info
Error: Command failed: wmic os get Caption
'wmic' is not recognized as an internal or external command,
operable program or batch file.


    at b (C:\Users\xxx\AppData\Local\npm-cache\_npx\c67e74de0542c87c\node_modules\envinfo\dist\envinfo.js:1:95303)
    at Function.e.exports.sync (C:\Users\xxx\AppData\Local\npm-cache\_npx\c67e74de0542c87c\node_modules\envinfo\dist\envinfo.js:1:97166)
    at e.exports (C:\Users\xxx\AppData\Local\npm-cache\_npx\c67e74de0542c87c\node_modules\envinfo\dist\envinfo.js:1:93520)
    at e.exports (C:\Users\xxx\AppData\Local\npm-cache\_npx\c67e74de0542c87c\node_modules\envinfo\dist\envinfo.js:1:92667)
    at C:\Users\xxx\AppData\Local\npm-cache\_npx\c67e74de0542c87c\node_modules\envinfo\dist\envinfo.js:1:91417
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async Promise.all (index 0) {
  code: 1,
  stdout: '',
  stderr: "'wmic' is not recognized as an internal or external command,\r\n" +
    'operable program or batch file.\r\n',
  failed: true,
  signal: null,
  cmd: 'wmic os get Caption',
  timedOut: false
}
npm notice

@erickriva
Copy link

erickriva commented Mar 3, 2022

There are some need of Powershell commands to replace wmic on browser.js:159 and ides.js:49?

ides.js:49: wmic datafile where name="C:\\\\Program Files\\\\Android\\\\Android Studio\\\\bin\\\\studio.exe" get Version
As Powershell command running through CMD, it probably will look like:
powershell -command "(Get-ItemProperty -LiteralPath 'C:\\\\Program Files\\\\Android\\\\Android Studio\\\\bin\\\\studio.exe').VersionInfo | Format-Table -Property FileVersion -HideTableHeaders"

browser.js:159: wmic datafile where "name='${explorerPath}'" get Version
As Powershell command running through CMD, it probably will look like:
powershell -command "(Get-ItemProperty -LiteralPath '${explorerPath}').VersionInfo | Format-Table -Property FileVersion -HideTableHeaders"

There are FileVersion and ProductVersion properties, and in my tests for these specific files it had same value.
Maybe -ExecutionPolicy Bypass is needed for Powershell scripts.

@HarryGwinnell
Copy link
Author

Thanks for the feedback @erickriva. I've made the changes as you suggested, but I've opted for ProductVersion, since FileVersion seems to include a version number of the build tooling which isn't relevant. It's interesting that the IE binary is still present in Win 11 despite being unable to open it.

For the execution policy, cycling through execution policy modes, I can't find a combination where Yarn will run but the command won't, so no concerns there.

Also bumped the version number by a major version

@NoahAndrews
Copy link

What's the status of this PR?

@tebeco
Copy link

tebeco commented Aug 16, 2024

🦤

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.

Wrong Version of Windows
5 participants