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

Move to ESM, update dependencies #683

Merged
merged 23 commits into from
Apr 3, 2023

Conversation

tommy-mitchell
Copy link
Collaborator

@tommy-mitchell tommy-mitchell commented Apr 1, 2023

Closes #601.

Refactors np into ESM format and updates dependencies, fixing breaking changes.

Some notes:

  • I've added a couple of ignore rules to xo to minimize diffs between updating xo's defaults. These can be removed if desired.
  • version.js is now implemented as a class, with previous exports being static methods. The previous default export was a function that constructed a new Version instance, but that's hard to replicate with ESM.
  • The async IIFE in cli-implementation.js has been removed in favor of a try/catch block with top-level await.
  • I've added an option to np in index.js to set the listr renderer, so the index tests can use the silent renderer
  • mockery, proxyquire, and execa_test_double have been dropped in favor of esmock and sinon
  • Tests have been simplified via macros and utils where possible
    • Take a look at test/_utils.js for how execa is being stubbed
  • Images have been moved from the repository root to a media/ folder

Todos:

  • Bump Node versions / other engines, if needed
    • I've only changed the Node version in package.json. Are we dropping old Node versions? Places to change:
      • package.json
      • readme.md
      • .github/workflows/main.yml
  • Update rxjs and other Observables dependencies
    • I'm not very familiar with either, so I opted to defer updating these by myself
  • Update integration test to use ESM
  • Resolve Support ESM only for the config file #599 - two tests fail due to cosmiconfig assuming that two .js fixtures are ESM configs when they're actually CJS

On the topic of updating dependencies, it might be worth moving to https://github.com/listr2/listr2 as an actively-maintained version of listr.

@tommy-mitchell
Copy link
Collaborator Author

I've gone ahead and bumped the Node workflows.

@tommy-mitchell
Copy link
Collaborator Author

Currently three tests are marked failing (integration and config issues). Everything else appears good.

@sindresorhus
Copy link
Owner

I've only changed the Node version in package.json. Are we dropping old Node versions? Places to change:

We can target Node.js 16.

On the topic of updating dependencies, it might be worth moving to https://github.com/listr2/listr2 as an actively-maintained version of listr.

👍

@tommy-mitchell
Copy link
Collaborator Author

tommy-mitchell commented Apr 2, 2023

We can target Node.js 16.

I've bumped the minimum Node and npm versions. Do the git and yarn versions need bumping too?

I think migrating to listr2 should be a separate PR.

@sindresorhus
Copy link
Owner

I've bumped the minimum Node and npm versions. Do the git and yarn versions need bumping too?

No

I think migrating to listr2 should be a separate PR.

👍

@tommy-mitchell
Copy link
Collaborator Author

tommy-mitchell commented Apr 2, 2023

A couple of solutions for the config issue:


Additionally, I'm going to drop async-exit-hook in favor of the async feature in exit-hook.

@sindresorhus
Copy link
Owner

Use a custom ESM loader with cosmiconfig:

Sure. With a todo comment to move to remove the workaround when the cosmiconfig thing is resolved.

Additionally, I'm going to drop async-exit-hook in favor of the async feature in exit-hook.

👍

@tommy-mitchell
Copy link
Collaborator Author

Updated. Some caveats:

  • cosmiconfig / Node can't load a .js file as ESM without a package.json with "type": "module" set. Explicitly using .mjs works. I've documented this in the readme.
  • The index tests give a SYNCHRONOUS TERMINATION NOTICE because the gracefulExit() handling is in cli-implementation.js. Using the np CLI still works, though.
    • On a related note, I'm not sure wait the minimumWait should be for the hook.

@tommy-mitchell
Copy link
Collaborator Author

I've also updated RxJS, which lets us drop @samverschueren/stream-to-observable, any-observable, and split.

@tommy-mitchell
Copy link
Collaborator Author

I think migrating to listr2 should be a separate PR.

This should wait until v6 is out of beta: listr2/listr2#666

@tommy-mitchell
Copy link
Collaborator Author

tommy-mitchell commented Apr 2, 2023

Update integration test to use ESM

I've gotten rid of the external submodule and changed the integration test to initialize an empty git repo in a subfolder for testing. I've tried to make these tests as cross-platform as possible. The test command is now:

xo && ava && ava test/integration.js --no-worker-threads

Since the integration test needs to use process.chdir(), it's run separately. AVA is configured to ignore it in the first run.


Currently, one of the integration tests is failing, but that can be fixed in #682.

@tommy-mitchell
Copy link
Collaborator Author

I've added a couple of ignore rules to xo to minimize diffs between updating xo's defaults. These can be removed if desired.

Do you want to enable these rules? comma-dangle and object-shorthand for method properties.

Other than that and the documented .js CJS vs ESM issue with cosmiconfig, this PR is good to go.

@sindresorhus
Copy link
Owner

This should wait until v6 is out of beta: listr2/listr2#666

Can you open an issue so we don't forget?

@sindresorhus
Copy link
Owner

Do you want to enable these rules? comma-dangle and object-shorthand for method properties.

Yes

@tommy-mitchell
Copy link
Collaborator Author

Done.

@sindresorhus sindresorhus merged commit 72879e0 into sindresorhus:main Apr 3, 2023
@sindresorhus
Copy link
Owner

Nice work 👍👍

Vylpes pushed a commit to Vylpes/random-bunny that referenced this pull request Sep 11, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [np](https://github.com/sindresorhus/np) | devDependencies | major | [`^7.7.0` -> `^8.0.0`](https://renovatebot.com/diffs/npm/np/7.7.0/8.0.4) |

---

### Release Notes

<details>
<summary>sindresorhus/np (np)</summary>

### [`v8.0.4`](https://github.com/sindresorhus/np/releases/tag/v8.0.4)

[Compare Source](sindresorhus/np@v8.0.3...v8.0.4)

-   Handle first time display of dependencies ([#&#8203;707](sindresorhus/np#707))  [`3f43d78`](sindresorhus/np@3f43d78)

### [`v8.0.3`](https://github.com/sindresorhus/np/releases/tag/v8.0.3)

[Compare Source](sindresorhus/np@v8.0.2...v8.0.3)

-   Fix skipping publish step ([#&#8203;706](sindresorhus/np#706))  [`51dcc2d`](sindresorhus/np@51dcc2d)

### [`v8.0.2`](https://github.com/sindresorhus/np/releases/tag/v8.0.2)

[Compare Source](sindresorhus/np@v8.0.1...v8.0.2)

-   Fix publish not working with Yarn  [`3d448c2`](sindresorhus/np@3d448c2)
-   Include stack trace in errors  [`12fce88`](sindresorhus/np@12fce88)

### [`v8.0.1`](https://github.com/sindresorhus/np/releases/tag/v8.0.1)

[Compare Source](sindresorhus/np@v8.0.0...v8.0.1)

-   Fix a crash in the new dependency check  [`beb7db1`](sindresorhus/np@beb7db1)

### [`v8.0.0`](https://github.com/sindresorhus/np/releases/tag/v8.0.0)

[Compare Source](sindresorhus/np@v7.7.0...v8.0.0)

##### Breaking

-   Require Node.js 16 ([#&#8203;683](sindresorhus/np#683))  [`72879e0`](sindresorhus/np@72879e0)

##### Improvements

-   Add 2FA support for npm version 9+ ([#&#8203;693](sindresorhus/np#693))  [`9cb4bfd`](sindresorhus/np@9cb4bfd)
-   Improve startup time ([#&#8203;688](sindresorhus/np#688))  [`eba203f`](sindresorhus/np@eba203f)
-   Improve the reliability of detecting which files will be included in the package ([#&#8203;682](sindresorhus/np#682))  [`a6ce792`](sindresorhus/np@a6ce792)
-   Add check for new dependencies ([#&#8203;681](sindresorhus/np#681))  [`6867fb9`](sindresorhus/np@6867fb9)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC43NC4yIiwidXBkYXRlZEluVmVyIjoiMzYuODkuMCIsInRhcmdldEJyYW5jaCI6ImRldmVsb3AifQ==-->

Reviewed-on: https://gitea.vylpes.xyz/RabbitLabs/random-bunny/pulls/67
Co-authored-by: Renovate Bot <renovate@vylpes.com>
Co-committed-by: Renovate Bot <renovate@vylpes.com>
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.

Move to ESM format Support ESM only for the config file
2 participants