chore: use tsup for building; package.json exports for better compat #33

bienzaaron · 2024-08-14T22:02:42Z

Swaps the existing build scripts out for tsup and sets package.json exports fields for the best compatibility with typescript.

I tested with the repro from the linked issue with moduleResolution modes: bundler, node, nodenext (plus module: nodenext as that's required), and node16 - all work with the updated exports field.

closes #32

bienzaaron · 2024-08-14T22:03:32Z

tsup.config.ts

+import { defineConfig } from "tsup";
+
+export default defineConfig({
+  outDir: "dist/lib",
+  entry: ["./lib/index.ts"],
+  format: ["cjs", "esm"],
+  target: "node18",
+  sourcemap: true,
+  clean: true,
+  minify: false,
+  shims: true,
+  dts: true,
+});


This can be moved into package.json if desired, but when in a typescript/javascript file, you get type hints from your editor, which is why I put it in its own config.

bienzaaron · 2024-08-14T22:04:06Z

package.json

  "scripts": {
    "prebuildify": "prebuildify --napi --strip",
    "install": "node-gyp-build",
-    "build": "npm run prebuildify && tsc && npm run gen-esm-wrapper",
+    "build": "npm run prebuildify && tsup",


tsc was removed, but with dts: true, tsup will call tsc.

benknoble · 2024-08-29T17:44:03Z

cc @jasnell what can we do to help move this forward?

benknoble · 2024-10-08T13:18:50Z

cc @mcollina any way to push this forward ?

mcollina · 2024-10-10T19:26:06Z

I currently do not have time

benknoble · 2024-10-10T20:37:30Z

@mcollina thanks for letting us know; do you know who else we can reach as maintainers that might have time to review this and move it forward?

mcollina · 2024-10-12T05:05:34Z

@metcoder95 ?

metcoder95 · 2024-10-12T07:42:34Z

👋

sorry for the delay, I oversight the notification. I will take a look at it this weekend and get back to you. thanks!

benknoble · 2024-10-13T00:08:58Z

Thank you all !!

metcoder95

Can resolve the conflicts?

After that, it lgtm

package.json

…into current

metcoder95 · 2024-10-20T09:16:21Z

Lint seems failing

bienzaaron · 2024-10-21T14:54:38Z

I had to change the export = {...} to get the tests passing. I was getting the error:

test/locks.ts 2> /Users/fsb984/work/oss/piscina-locks/node_modules/ts-node/src/index.ts:859
test/locks.ts 2>     return new TSError(diagnosticText, diagnosticCodes, diagnostics);
test/locks.ts 2>            ^
test/locks.ts 2> TSError: ⨯ Unable to compile TypeScript:
test/locks.ts 2> test/locks.ts(6,3): error TS2614: Module '".."' has no exported member 'request'. Did you mean to use 'import request from ".."' instead?
test/locks.ts 2> test/locks.ts(7,3): error TS2614: Module '".."' has no exported member 'query'. Did you mean to use 'import query from ".."' instead?
test/locks.ts 2> test/locks.ts(8,3): error TS2614: Module '".."' has no exported member 'version'. Did you mean to use 'import version from ".."' instead?
test/locks.ts 2>
test/locks.ts 2>     at createTSError (/Users/fsb984/work/oss/piscina-locks/node_modules/ts-node/src/index.ts:859:12)
test/locks.ts 2>     at reportTSError (/Users/fsb984/work/oss/piscina-locks/node_modules/ts-node/src/index.ts:863:19)
test/locks.ts 2>     at getOutput (/Users/fsb984/work/oss/piscina-locks/node_modules/ts-node/src/index.ts:1077:36)
test/locks.ts 2>     at Object.compile (/Users/fsb984/work/oss/piscina-locks/node_modules/ts-node/src/index.ts:1433:41)
test/locks.ts 2>     at Module.m._compile (/Users/fsb984/work/oss/piscina-locks/node_modules/ts-node/src/index.ts:1617:30)
test/locks.ts 2>     at module.exports (/Users/fsb984/work/oss/piscina-locks/node_modules/default-require-extensions/js.js:7:9)
test/locks.ts 2>     at /Users/fsb984/work/oss/piscina-locks/node_modules/append-transform/index.js:64:4
test/locks.ts 2>     at require.extensions.<computed> (/Users/fsb984/work/oss/piscina-locks/node_modules/ts-node/src/index.ts:1621:12)
test/locks.ts 2>     at Object.<anonymous> (/Users/fsb984/work/oss/piscina-locks/node_modules/append-transform/index.js:64:4)
test/locks.ts 2>     at Module.load (node:internal/modules/cjs/loader:1288:32) {
test/locks.ts 2>   diagnosticCodes: [ 2614, 2614, 2614 ]
test/locks.ts 2> }

This should be mostly equivalent, except a default key is added to the CJS version.
with test.cjs:

const def = require('.');
console.log(def);

const { request, query, version } = require('.');
console.log(request, query, version);

and test.mjs:

import def from "./dist/lib/index.mjs";
import { query, request, version } from "./dist/lib/index.mjs";
console.log(def);
console.log(request, query, version);

before this change:

> node test.cjs
{
  request: [AsyncFunction: request],
  query: [Function: query],
  version: '3.0.0'
}
[AsyncFunction: request] [Function: query] 3.0.0

> node test.mjs 
{
  request: [AsyncFunction: request],
  query: [Function: query],
  version: '3.0.0'
}
[AsyncFunction: request] [Function: query] 3.0.0

after this change:

> node test.mjs
{
  request: [AsyncFunction: request],
  query: [Function: query],
  version: '3.0.0'
}
[AsyncFunction: request] [Function: query] 3.0.0

> node test.cjs
{
  default: [Getter],
  query: [Getter],
  request: [Getter],
  version: [Getter]
}
[AsyncFunction: request] [Function: query] 3.0.0

metcoder95 · 2024-10-21T21:07:45Z

No worries, that should be fine

benknoble · 2024-10-21T21:31:54Z

Now there are some C++ compilation errors? Hm

metcoder95 · 2024-10-22T06:44:55Z

I'll take a look later Today

bienzaaron · 2024-10-23T20:34:18Z

Okay, this one should go through 😅 it was pulling in node 23 headers, which was causing problems. Didn't happen on my machine, because it's a corporate device and it doesn't have access to 23 yet. I added a --target argument matching the node version that is currently installed (so it would match the github actions version, or whatever is installed locally). I can update that to whatever version, but it does appear to need to be an exact version - just 22 won't work, for example.

metcoder95 · 2024-10-24T06:47:10Z

Thanks, that should solve it, now there's an issue with the artifacts we upload; let me fix it so you can rebase with current

metcoder95 · 2024-10-24T06:55:20Z

Can you update your branch with current?

…into current

metcoder95 · 2024-10-25T08:51:55Z

The command doesn't work for windows bc of the subshell.

bienzaaron added 2 commits August 14, 2024 17:33

use tsup for building; package.json exports for max compat

10829db

slight correction

349f79b

bienzaaron commented Aug 14, 2024

View reviewed changes

make sure we keep package.json

483239d

metcoder95 reviewed Oct 13, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

bienzaaron added 2 commits October 18, 2024 11:33

Merge branch 'current' of https://github.com/piscinajs/piscina-locks …

fdd5f61

…into current

move onSuccess hook into tsup config

a42cd15

bienzaaron requested a review from metcoder95 October 18, 2024 15:40

metcoder95 approved these changes Oct 20, 2024

View reviewed changes

bienzaaron added 2 commits October 21, 2024 10:36

fix lint

362633d

more compatible export

f4f0788

bienzaaron requested a review from metcoder95 October 21, 2024 15:01

add target to prebuildify

de5b482

Merge branch 'current' of https://github.com/piscinajs/piscina-locks …

7878618

…into current

metcoder95 changed the title ~~use tsup for building; package.json exports for better compat~~ chore: use tsup for building; package.json exports for better compat Oct 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: use tsup for building; package.json exports for better compat #33

chore: use tsup for building; package.json exports for better compat #33

bienzaaron commented Aug 14, 2024

bienzaaron Aug 14, 2024

bienzaaron Aug 14, 2024

benknoble commented Aug 29, 2024

benknoble commented Oct 8, 2024

mcollina commented Oct 10, 2024

benknoble commented Oct 10, 2024

mcollina commented Oct 12, 2024

metcoder95 commented Oct 12, 2024

benknoble commented Oct 13, 2024

metcoder95 left a comment

metcoder95 commented Oct 20, 2024

bienzaaron commented Oct 21, 2024

metcoder95 commented Oct 21, 2024

benknoble commented Oct 21, 2024

metcoder95 commented Oct 22, 2024

bienzaaron commented Oct 23, 2024 •

edited

Loading

metcoder95 commented Oct 24, 2024

metcoder95 commented Oct 24, 2024

metcoder95 commented Oct 25, 2024

chore: use tsup for building; package.json exports for better compat #33

Are you sure you want to change the base?

chore: use tsup for building; package.json exports for better compat #33

Conversation

bienzaaron commented Aug 14, 2024

bienzaaron Aug 14, 2024

Choose a reason for hiding this comment

bienzaaron Aug 14, 2024

Choose a reason for hiding this comment

benknoble commented Aug 29, 2024

benknoble commented Oct 8, 2024

mcollina commented Oct 10, 2024

benknoble commented Oct 10, 2024

mcollina commented Oct 12, 2024

metcoder95 commented Oct 12, 2024

benknoble commented Oct 13, 2024

metcoder95 left a comment

Choose a reason for hiding this comment

metcoder95 commented Oct 20, 2024

bienzaaron commented Oct 21, 2024

metcoder95 commented Oct 21, 2024

benknoble commented Oct 21, 2024

metcoder95 commented Oct 22, 2024

bienzaaron commented Oct 23, 2024 • edited Loading

metcoder95 commented Oct 24, 2024

metcoder95 commented Oct 24, 2024

metcoder95 commented Oct 25, 2024

bienzaaron commented Oct 23, 2024 •

edited

Loading