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

Separate index.js and test.js into different files #234

Merged
merged 15 commits into from
Mar 30, 2023

Conversation

tommy-mitchell
Copy link
Contributor

@tommy-mitchell tommy-mitchell commented Mar 28, 2023

Per #233.

Tasks:

  • Target Node 14
  • Update dependencies, fix breaking changes
  • Separate into different source files
  • Separate tests, consolidate via macros
  • Add doc comments -> separate PR

Miscellaneous fixes:

  • Fixed extraneous indents in fixtures
  • Fixed extraneous leading space in readme
  • Fixed test dealing with process.title, added note to readme/index.d.ts about overriding options.pkg (needs better wording)

@tommy-mitchell tommy-mitchell mentioned this pull request Mar 28, 2023
9 tasks
readme.md Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

This looks good so far 👍

@sindresorhus
Copy link
Owner

sindresorhus commented Mar 29, 2023

You can target Node.js 16.


Here's what we may be able to take advantage of:

Node.js 16

  • private class methods and accessors
  • import {setTimeout} from 'node:timers/promises';
  • import stream from 'node:stream/promises';
  • Native AggregateError.
  • String.prototype.replaceAll
  • Promise.any
  • Array/String#at (Node v16.6.0)
  • Object.hasOwn (v16.10)

Node.js 14

  • private class fields
  • Top-level await
    • Get rid of all (async () => {})()
  • crypto.randomInt (Node.js 14.10)
  • Optional chaining
  • Nullish Coalescing
  • import fs from 'node:fs/promises';
  • crypto.randomUUID() (Node.js 14.17)
  • AbortController signal option (Node.js 14.18)

@tommy-mitchell
Copy link
Contributor Author

tommy-mitchell commented Mar 29, 2023

You can target Node.js 16.

I'll do this in a separate PR then, the diff for this one is already pretty large and I'd like the changes from the improvements to be easier to see.

By the way, is this list posted anywhere else? It's a nice checklist for a project bumping their Node version.

const parserOptions = buildParserOptions(parsedOptions);
const result = buildResult(parsedOptions, parserOptions);

process.title = result.pkg.bin ? Object.keys(result.pkg.bin)[0] : result.pkg.name;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Related to

added note to readme/index.d.ts about overriding options.pkg

this check doesn't take into account that, if options.pkg was overridden, pkg.name might not exist

@sindresorhus
Copy link
Owner

By the way, is this list posted anywhere else? It's a nice checklist for a project bumping their Node version.

It's just my personal list. I think the Node.js team may post something similar on new major versions.

@sindresorhus
Copy link
Owner

I'll do this in a separate PR then, the diff for this one is already pretty large and I'd like the changes from the improvements to be easier to see.

👍

@tommy-mitchell
Copy link
Contributor Author

I've split up associated tests from test.js into their own files and consolidated via macros or utils where possible, especially for handling fixtures. Some more could be done here, but I think it's good enough for now.

@tommy-mitchell tommy-mitchell marked this pull request as ready for review March 29, 2023 19:01
@tommy-mitchell tommy-mitchell changed the title Separate index.js into different source files Separate index.js and test.js into different files Mar 29, 2023
@tommy-mitchell tommy-mitchell mentioned this pull request Mar 29, 2023
@sindresorhus
Copy link
Owner

Can you fix the merge conflict?

@tommy-mitchell
Copy link
Contributor Author

Should be good now

@sindresorhus sindresorhus merged commit a94476a into sindresorhus:main Mar 30, 2023
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