feat: convert to typescript and esm by lukekarrys · Pull Request #77 · npm/read

lukekarrys · 2023-10-31T20:37:21Z

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

test/fixtures/setup.ts

lukekarrys · 2023-10-31T20:43:34Z

@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?

isaacs

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

test/basic.ts

package.json

test/fixtures/setup.ts

lukekarrys · 2023-11-01T14:16:13Z

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 · 2023-11-01T18:46:42Z

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

lukekarrys · 2023-11-01T18:52:35Z

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

test/fixtures/setup.ts

+import { spawn } from 'child_process'
+import { fileURLToPath } from 'url'
+
+const esc = (str: string) => str.replace(/([()])/g, '\\$1')


wraithgar · 2023-11-03T18:47:44Z

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 · 2023-11-16T14:50:34Z

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

scripts/template-oss/index.js

src/read.ts

lukekarrys · 2023-11-16T15:28:55Z

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

Important changes (npm/read#77): - exported as ESM - includes type definitions - no callback API (npm/read#40) It was possible to recreate the callback API using `Promise.then`, but it's not really worth it in this case.

lukekarrys changed the title ~~lk/isaacs/main ts~~ Convert to TypeScript Hybrid CJS + ESM Oct 31, 2023

github-advanced-security bot found potential problems Oct 31, 2023

View reviewed changes

test/fixtures/setup.ts Fixed Show fixed Hide fixed

test/fixtures/setup.ts Fixed Show fixed Hide fixed

lukekarrys mentioned this pull request Oct 31, 2023

RFC: TypeScript + ESM/CJS Hybrid Modules with tshy npm/template-oss#369

Closed

isaacs suggested changes Oct 31, 2023

View reviewed changes

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

github-advanced-security bot found potential problems Oct 31, 2023

View reviewed changes

test/fixtures/setup.ts Fixed Show fixed Hide fixed

test/fixtures/setup.ts Fixed Show fixed Hide fixed

lukekarrys force-pushed the lk/isaacs/main-ts branch from 14b6e6e to 8b4d28c Compare October 31, 2023 21:43

lukekarrys mentioned this pull request Nov 1, 2023

Engines declaration is incorrect. isaacs/resolve-import#4

Closed

lukekarrys force-pushed the lk/isaacs/main-ts branch from ae6580e to bec6462 Compare November 3, 2023 17:03

github-advanced-security bot found potential problems Nov 3, 2023

View reviewed changes

test/fixtures/setup.ts

import { spawn } from 'child_process'

import { fileURLToPath } from 'url'

const esc = (str: string) => str.replace(/([()])/g, '\\$1')

Check failure

Code scanning / CodeQL

Incomplete string escaping or encoding

This does not escape backslash characters in the input.

lukekarrys marked this pull request as ready for review November 3, 2023 17:05

lukekarrys requested a review from a team as a code owner November 3, 2023 17:05

lukekarrys force-pushed the lk/isaacs/main-ts branch 6 times, most recently from 2702dc8 to d604cc1 Compare November 6, 2023 17:29

lukekarrys mentioned this pull request Nov 6, 2023

Packages should be able to be written in TypeScript + published as ESM+CJS npm/statusboard#738

Closed

lukekarrys force-pushed the lk/isaacs/main-ts branch 4 times, most recently from d3d2501 to 1e61bc0 Compare November 6, 2023 18:35

lukekarrys marked this pull request as draft November 6, 2023 18:36

lukekarrys force-pushed the lk/isaacs/main-ts branch from 1e61bc0 to bc4337e Compare November 6, 2023 18:38

lukekarrys marked this pull request as ready for review November 16, 2023 03:01

lukekarrys force-pushed the lk/isaacs/main-ts branch 2 times, most recently from 4ba826a to 51e6b4d Compare November 16, 2023 03:19

lukekarrys marked this pull request as draft November 16, 2023 03:19

lukekarrys force-pushed the lk/isaacs/main-ts branch from 51e6b4d to dbdeaf2 Compare November 16, 2023 03:21

lukekarrys changed the title ~~Convert to TypeScript Hybrid CJS + ESM~~ feat: convert to typescript and esm Nov 16, 2023

lukekarrys marked this pull request as ready for review November 16, 2023 14:49

feat: convert to typescript and esm

c53ac8d

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.

lukekarrys force-pushed the lk/isaacs/main-ts branch from dbdeaf2 to c53ac8d Compare November 16, 2023 14:51

wraithgar reviewed Nov 16, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

wraithgar reviewed Nov 16, 2023

View reviewed changes

scripts/template-oss/index.js Show resolved Hide resolved

wraithgar reviewed Nov 16, 2023

View reviewed changes

src/read.ts Show resolved Hide resolved

lukekarrys added 2 commits November 16, 2023 08:25

chore: bump dev deps

fa07e1d

fix: add TODOs for all ignored coverage

d559f79

wraithgar approved these changes Nov 16, 2023

View reviewed changes

lukekarrys merged commit 18cb7bd into main Nov 16, 2023

lukekarrys deleted the lk/isaacs/main-ts branch November 16, 2023 15:52

github-actions bot mentioned this pull request Nov 16, 2023

chore: release 3.0.0 #60

Merged

Conversation

lukekarrys commented Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukekarrys commented Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isaacs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukekarrys commented Nov 1, 2023

Uh oh!

isaacs commented Nov 1, 2023

Uh oh!

lukekarrys commented Nov 1, 2023

Uh oh!

Check failure

wraithgar commented Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukekarrys commented Nov 16, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukekarrys commented Nov 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lukekarrys commented Oct 31, 2023 •

edited

Loading

lukekarrys commented Oct 31, 2023 •

edited

Loading

wraithgar commented Nov 3, 2023 •

edited

Loading