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

feat: convert to typescript and esm #77

Merged
merged 3 commits into from
Nov 16, 2023
Merged

feat: convert to typescript and esm #77

merged 3 commits into from
Nov 16, 2023

Conversation

lukekarrys
Copy link
Contributor

@lukekarrys lukekarrys commented Oct 31, 2023

BREAKING CHANGE: read is now written in TypeScript and types are now
shipped as part of this package.

BREAKING CHANGE: read now only exports a named export { read }
and does not have a default export.


TODO:

  • Set required template-oss configs so it template-oss-check passes
  • Get eslint import config working with typescript + .js files (or use allowImportingTsExtensions)
  • Add typescript eslint plugin
  • Update engines
  • Use TypeScript, ESM, tap18 support template-oss#371 instead of local tarball

@lukekarrys lukekarrys changed the title lk/isaacs/main ts Convert to TypeScript Hybrid CJS + ESM Oct 31, 2023
test/fixtures/setup.ts Fixed Show fixed Hide fixed
test/fixtures/setup.ts Fixed Show fixed Hide fixed
@lukekarrys
Copy link
Contributor Author

lukekarrys commented Oct 31, 2023

@isaacs any thoughts on the tsconfig.json or tshy+tap setup would be greatly appreciated. this is based on #44 and updated to replace the previous ts+esm/cjs config with tshy and tap@18.

also does it need a separate read.d.ts file if the types are exported from read.ts?

Copy link
Contributor

@isaacs isaacs left a comment

Choose a reason for hiding this comment

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

Some changes suggested.

And yes, definitely remove the read.d.ts if this is being written in TS, it will be extraneous and just serve to confuse things. Better to let TS find the appropriate dialect-specific types in dist.

src/read.ts Outdated Show resolved Hide resolved
src/read.ts Outdated Show resolved Hide resolved
src/read.ts Outdated Show resolved Hide resolved
test/basic.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
test/fixtures/setup.ts Fixed Show fixed Hide fixed
test/fixtures/setup.ts Fixed Show fixed Hide fixed
@lukekarrys
Copy link
Contributor Author

Made all the changes suggested. I was not set on a default export, and I think TypeScript+ESM is a good push for us to end the pattern module.exports = () => {}; module.exports.x = 1;

@isaacs
Copy link
Contributor

isaacs commented Nov 1, 2023

Those node 14 tests are never going to pass with tap 18, probably best to remove from the matrix.

@lukekarrys
Copy link
Contributor Author

yes node 14 is definitely getting dropped as part of this PR

test/fixtures/setup.ts Dismissed Show dismissed Hide dismissed
@lukekarrys lukekarrys marked this pull request as ready for review November 3, 2023 17:05
@lukekarrys lukekarrys requested a review from a team as a code owner November 3, 2023 17:05
@wraithgar
Copy link
Member

wraithgar commented Nov 3, 2023

If we're bumping engines let's bump engines

npm view npm engines
{ node: '^18.17.0 || >=20.5.0' }
$ npm query '#read' |json -a _id from
read@2.1.0 [
  "node_modules/init-package-json",
  "node_modules/promzard",
  "workspaces/libnpmexec",
  ""
]

@lukekarrys lukekarrys force-pushed the lk/isaacs/main-ts branch 2 times, most recently from 4ba826a to 51e6b4d Compare November 16, 2023 03:19
@lukekarrys lukekarrys marked this pull request as draft November 16, 2023 03:19
@lukekarrys lukekarrys changed the title Convert to TypeScript Hybrid CJS + ESM feat: convert to typescript and esm Nov 16, 2023
@lukekarrys lukekarrys marked this pull request as ready for review November 16, 2023 14:49
@lukekarrys
Copy link
Contributor Author

This is now using @npmcli/template-oss@4.20.0 and has been squashed down to single commit.

@wraithgar It also removes the engines update entirely as I dropped it back to tap@16 + c8 and ts-node to avoid that issue for now.

BREAKING CHANGE: `read` is now written in TypeScript and types are now
shipped as part of this package.

BREAKING CHANGE: `read` now only exports a named export `{ read }`
and does not have a default export.
package.json Outdated Show resolved Hide resolved
src/read.ts Show resolved Hide resolved
@lukekarrys
Copy link
Contributor Author

Bumped out of date devDeps and adding TODO comments for all ignored coverage.

@lukekarrys lukekarrys merged commit 18cb7bd into main Nov 16, 2023
26 checks passed
@lukekarrys lukekarrys deleted the lk/isaacs/main-ts branch November 16, 2023 15:52
@github-actions github-actions bot mentioned this pull request Nov 16, 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.

None yet

3 participants