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

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

Merged
merged 10 commits into from
Dec 22, 2024

Conversation

bienzaaron
Copy link
Contributor

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

tsup.config.ts Outdated
Comment on lines 1 to 13
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,
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

package.json Outdated
"scripts": {
"prebuildify": "prebuildify --napi --strip",
"install": "node-gyp-build",
"build": "npm run prebuildify && tsc && npm run gen-esm-wrapper",
"build": "npm run prebuildify && tsup",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@benknoble
Copy link

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

@benknoble
Copy link

cc @mcollina any way to push this forward ?

@mcollina
Copy link
Contributor

I currently do not have time

@benknoble
Copy link

@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
Copy link
Contributor

@metcoder95 ?

@metcoder95
Copy link
Member

👋

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

@benknoble
Copy link

Thank you all !!

Copy link
Member

@metcoder95 metcoder95 left a comment

Choose a reason for hiding this comment

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

Can resolve the conflicts?

After that, it lgtm

package.json Outdated Show resolved Hide resolved
@bienzaaron bienzaaron requested a review from metcoder95 October 18, 2024 15:40
@metcoder95
Copy link
Member

Lint seems failing

@bienzaaron
Copy link
Contributor Author

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

@bienzaaron bienzaaron requested a review from metcoder95 October 21, 2024 15:01
@metcoder95
Copy link
Member

No worries, that should be fine

@benknoble
Copy link

Now there are some C++ compilation errors? Hm

@metcoder95
Copy link
Member

I'll take a look later Today

@bienzaaron
Copy link
Contributor Author

bienzaaron commented Oct 23, 2024

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
Copy link
Member

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
Copy link
Member

Can you update your branch with current?

@metcoder95 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
@metcoder95
Copy link
Member

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

@metcoder95
Copy link
Member

Sorry for the extremely long reply, completely missed the PR;
let me take it back soon on this

@metcoder95 metcoder95 merged commit ebfe72f into piscinajs:current Dec 22, 2024
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.

Exports for typescript may not be correct
4 participants