fix(build): switch to esbuild and tsc from pika #345

kfcampbell · 2023-06-05T18:00:52Z

Replicating octokit/request.js#584 to another repo.

Pika has been deprecated for a while now, and the project has now been archived, and because of that the released npm package is missing most of the files generated by the build step.

Uses esbuild to transpile the TS source code into an ESM source, NodeJs bundle, and a browser bundle

Uses the TypeScript compiler to generate the types.

Behavior

Before the change?

Uses pika for the build system
The released npm package is missing most of the files generated by the build step. dist-node, dist-types, dist-web... even though they are generated correctly.

After the change?

Uses esbuild to generate the JS bundles
Uses the TypeScript compiler to generate the type definitions
The correct file matching patterns are used so we publish all the necessary files again.

Additional info

Pull request checklist

Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)
Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

Yes (Please add the Type: Breaking change label)
No

If Yes, what's the impact:

N/A

Pull request type

Please add the corresponding label for change this PR introduces:

Bugfix: Type: Bug
Dependencies/code cleanup: Type: Maintenance

nickfloyd

🎉

wolfy1339 · 2023-06-05T18:15:04Z

Don't forget to mark all type imports and exports

gr2m · 2023-06-05T21:50:01Z

Don't forget to mark all type imports and exports

I've seen this causing a few times. Do you know if we can somehow add a CI check to make sure we don't miss these in future?

kfcampbell · 2023-06-05T21:56:15Z

@wolfy1339 I believe I've done this correctly now; please take another look!

@gr2m we could probably use something like this eslint rule. Without taking an extra dependency, perhaps it'd be better to use --verbatimModuleSyntax when compiling. Thoughts?

gr2m · 2023-06-05T21:59:15Z

@gr2m we could probably use something like this eslint rule. Without taking an extra dependency, perhaps it'd be better to use --verbatimModuleSyntax when compiling. Thoughts?

yeah I would like to avoid adding eslint

--verbatimModuleSyntax looks great, I didn't know about this flag! It seems like the only gotcha about it is for modules with side effect, which we have been careful to avoid. I'd say let's add it to all the things

wolfy1339 · 2023-06-05T22:02:52Z

I agree that the typescript build option looks good.

However I'm not sure if it will have an impact since esbuild doesn't use typescript to compile.

kfcampbell · 2023-06-05T22:18:21Z

👍 on adding the build flag; I've done so in the latest commit. When the type keyword is removed from imports (here for example), the tsc call in our npm run build command throws the following errors:

 sh$ npm run build

> @octokit/[email protected] build /home/kfcampbell/github/dev/auth-token.js
> node scripts/build.mjs && tsc -p tsconfig.json --verbatimModuleSyntax

src/hook.ts:2:3 - error TS1484: 'AnyResponse' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.

2   AnyResponse,
    ~~~~~~~~~~~

src/hook.ts:3:3 - error TS1484: 'EndpointDefaults' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.

3   EndpointDefaults,
    ~~~~~~~~~~~~~~~~

src/hook.ts:4:3 - error TS1484: 'EndpointOptions' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.

4   EndpointOptions,
    ~~~~~~~~~~~~~~~

src/hook.ts:5:3 - error TS1484: 'RequestInterface' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.

5   RequestInterface,
    ~~~~~~~~~~~~~~~~

src/hook.ts:6:3 - error TS1484: 'RequestParameters' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.

6   RequestParameters,
    ~~~~~~~~~~~~~~~~~

src/hook.ts:7:3 - error TS1484: 'Route' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.

7   Route,
    ~~~~~

src/hook.ts:8:3 - error TS1484: 'Token' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.

8   Token,
    ~~~~~


Found 7 errors in the same file, starting at: src/hook.ts:2

npm ERR! code ELIFECYCLE
npm ERR! errno 2
npm ERR! @octokit/[email protected] build: `node scripts/build.mjs && tsc -p tsconfig.json --verbatimModuleSyntax`
npm ERR! Exit status 2
npm ERR!
npm ERR! Failed at the @octokit/[email protected] build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/kfcampbell/.npm/_logs/2023-06-05T22_15_33_319Z-debug.log

gr2m · 2023-06-05T22:21:52Z

That's great! I'd add it as an option to https://github.com/octokit/tsconfig and maybe release it as a breaking change so that the different packages don't suddenly start breaking. We can then address all type import errors like show in your comment as we update to the new breaking version of @octokit/tsconfig

wolfy1339 · 2023-06-05T22:25:49Z

While we're at it, we should move the options that are common across the repos to @octokit/tsconfig

wolfy1339 · 2023-06-05T23:03:15Z

👍 on adding the build flag; I've done so in the latest commit. When the type keyword is removed from imports (here for example), the tsc call in our npm run build command throws the following errors:

 sh$ npm run build

> @octokit/[email protected] build /home/kfcampbell/github/dev/auth-token.js
> node scripts/build.mjs && tsc -p tsconfig.json --verbatimModuleSyntax

src/hook.ts:2:3 - error TS1484: 'AnyResponse' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.

2   AnyResponse,
    ~~~~~~~~~~~

src/hook.ts:3:3 - error TS1484: 'EndpointDefaults' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.

3   EndpointDefaults,
    ~~~~~~~~~~~~~~~~

src/hook.ts:4:3 - error TS1484: 'EndpointOptions' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.

4   EndpointOptions,
    ~~~~~~~~~~~~~~~

src/hook.ts:5:3 - error TS1484: 'RequestInterface' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.

5   RequestInterface,
    ~~~~~~~~~~~~~~~~

src/hook.ts:6:3 - error TS1484: 'RequestParameters' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.

6   RequestParameters,
    ~~~~~~~~~~~~~~~~~

src/hook.ts:7:3 - error TS1484: 'Route' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.

7   Route,
    ~~~~~

src/hook.ts:8:3 - error TS1484: 'Token' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.

8   Token,
    ~~~~~


Found 7 errors in the same file, starting at: src/hook.ts:2

npm ERR! code ELIFECYCLE
npm ERR! errno 2
npm ERR! @octokit/[email protected] build: `node scripts/build.mjs && tsc -p tsconfig.json --verbatimModuleSyntax`
npm ERR! Exit status 2
npm ERR!
npm ERR! Failed at the @octokit/[email protected] build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/kfcampbell/.npm/_logs/2023-06-05T22_15_33_319Z-debug.log

That doesn't catch the ones which aren't types but are used as types, like when using a class for the type of a parameter in a function

package.json

kfcampbell · 2023-06-06T19:17:37Z

I'd add it as an option to https://github.com/octokit/tsconfig and maybe release it as a breaking change so that the different packages don't suddenly start breaking. We can then address all type import errors like show in your comment as we update to the new breaking version of @octokit/tsconfig

@gr2m that seems reasonable! I've created octokit/tsconfig#15 to do so. Once that PR is merged and released, I can update all the in-flight pika removal PRs to bump octokit/tsconfig versions as well.

That doesn't catch the ones which aren't types but are used as types, like when using a class for the type of a parameter in a function

@wolfy1339 if you can find a good way to do this, please let me know and I can help roll that out as well! Thanks for all the thoughts so far.

kfcampbell · 2023-06-06T19:23:31Z

While we're at it, we should move the options that are common across the repos to @octokit/tsconfig

Some other options that may be reasonable to move to octokit/tsconfig are:

  "compilerOptions": {
    "esModuleInterop": true,
    "declaration": true,
    "emitDeclarationOnly": true,
    "sourceMap": true,
  },
  "include": ["src/**/*"]

Do either of you have thoughts on these or others before I add them to octokit/tsconfig?

kfcampbell · 2023-06-06T19:56:24Z

Note that as of 9a3587f, tests are failing in Node 16 with errors like:

    test/index.test.ts:1:10 - error TS1286: ESM syntax is not allowed in a CommonJS module when 'verbatimModuleSyntax' is enabled.

    1 import { request } from "@octokit/request";

I've resolved these instances by using the CommonJS syntax const { request } = require("@octokit/request");

However, we're also getting an issue in index.ts:

 FAIL  test/index.test.ts
  ● Test suite failed to run

    src/index.ts:11:1 - error TS1287: A top-level 'export' modifier cannot be used on value declarations in a CommonJS module when 'verbatimModuleSyntax' is enabled.

    11 export const createTokenAuth: StrategyInterface = function createTokenAuth(
       ~~~~~~

We could use an intermediary object in exports, though that would be a breaking change for consumers. Switching to ESM would be an option, though that's a whole other effort we've been postponing for some time. Are there other options I'm missing here to export this correctly?

wolfy1339 · 2023-06-06T20:03:50Z

I think we can add a tsconfig just for tests that disables that option

kfcampbell · 2023-06-06T20:12:20Z

I'm picturing changing package.json to read something like:

...
  "scripts": {
    "build": "node scripts/build.mjs && tsc -p tsconfig.json",
    "test": "node scripts/build.mjs && tsc -p tsconfig.test.json && jest --coverage",
...

I don't love the idea of splitting the files but it's probably better than avoiding the compiler option entirely, especially with how easy it is (for me) to forget about type exports. Is that also what you're picturing?

wolfy1339 · 2023-06-06T20:50:19Z

I'm picturing changing package.json to read something like:
...
  "scripts": {
    "build": "node scripts/build.mjs && tsc -p tsconfig.json",
    "test": "node scripts/build.mjs && tsc -p tsconfig.test.json && jest --coverage",
...
I don't love the idea of splitting the files but it's probably better than avoiding the compiler option entirely, especially with how easy it is (for me) to forget about type exports. Is that also what you're picturing?

You don't need to change the scripts at all.
The only change that could be necessary is to the jest config to point it to the correct tsconfig

kfcampbell · 2023-06-06T22:20:40Z

@wolfy1339 thanks for the help! It's now done. I've also bumped the major version of octokit/tsconfig and removed the explicit specification of verbatimModuleSyntax. Please take another look!

wolfy1339

Would it be wise to move it into the test directory?

As done in @octokit/webhooks:
https://github.com/octokit/webhooks.js/blob/bc0ab962cb6218b6714c594a4b1c16ec8acbb707/tsconfig.json

tsconfig.test.json

wolfy1339 · 2023-06-06T22:25:25Z

tsconfig.test.json

+    "sourceMap": true,
+    "verbatimModuleSyntax": false
+  },
+  "include": ["src/**/*"]


Suggested change

"include": ["src/**/*"]

"include": ["test/**/*"]

This one I'm less sure about.

I don't think we should take this: then the tsconfig would only apply to the single test file, not the source code. Perhaps include should be something like **/* if we want to include both the test file(s) and the source code in the test tsconfig.

Co-authored-by: wolfy1339 <[email protected]>

kfcampbell · 2023-06-06T22:47:26Z

Would it be wise to move it into the test directory?

Good call! Done.

kfcampbell · 2023-06-06T22:50:11Z

Once merged, any reservations to me rolling out the pattern introduced on this PR to the other octokit modules using pika?

github-actions · 2023-06-06T23:10:41Z

🎉 This PR is included in version 3.0.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Small fixups

Switch to esbuild and tsc from pika

6b8c7e4

kfcampbell added the Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR label Jun 5, 2023

kfcampbell requested review from nickfloyd and wolfy1339 June 5, 2023 18:00

Fix linting errors

c93da37

nickfloyd approved these changes Jun 5, 2023

View reviewed changes

kfcampbell added 6 commits June 5, 2023 14:33

Merge branch 'main' into switch-to-esbuild

c771f50

Explicitly mark imports

6e83fdd

Undo overzealous type import

04d5f09

npm i --save-dev @octokit/types

e2790b9

Use node 14 to create lockfile

0cb4d2b

Redo appropriately zealous type import

d29f2d3

wolfy1339 changed the title ~~Switch to esbuild and tsc from pika~~ fix(build): switch to esbuild and tsc from pika Jun 5, 2023

Add --verbatimModuleSyntax build option

b244ba2

wolfy1339 reviewed Jun 6, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

Move verbatimModuleSyntax to tsconfig

9a3587f

kfcampbell mentioned this pull request Jun 6, 2023

Set verbatimModuleSyntax to true octokit/tsconfig#15

Merged

kfcampbell added 2 commits June 6, 2023 15:15

Create tsconfig.test.json

e644e94

Bump version of octokit/tsconfig

01a7dc0

wolfy1339 reviewed Jun 6, 2023

View reviewed changes

kfcampbell and others added 2 commits June 6, 2023 15:42

Update tsconfig.test.json

2541524

Co-authored-by: wolfy1339 <[email protected]>

Move tsconfig.test.json to test directory

4d6cd2a

wolfy1339 approved these changes Jun 6, 2023

View reviewed changes

kfcampbell merged commit eaf16ab into main Jun 6, 2023

kfcampbell deleted the switch-to-esbuild branch June 6, 2023 23:09

github-actions bot added the released label Jun 6, 2023

wolfy1339 pushed a commit that referenced this pull request Jun 7, 2023

build: small fixup follow-up to #345 (#346)

fdf91b8

Small fixups

NOUIY mentioned this pull request May 24, 2024

[Snyk] Upgrade @octokit/auth-token from 3.0.0 to 3.0.4 NOUIY/core.js#159

Open

NOUIY mentioned this pull request Sep 16, 2024

[Snyk] Upgrade: , universal-user-agent, , , , , before-after-hook NOUIY/core.js#164

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(build): switch to esbuild and tsc from pika #345

fix(build): switch to esbuild and tsc from pika #345

kfcampbell commented Jun 5, 2023

nickfloyd left a comment

wolfy1339 commented Jun 5, 2023

gr2m commented Jun 5, 2023

kfcampbell commented Jun 5, 2023

gr2m commented Jun 5, 2023

wolfy1339 commented Jun 5, 2023

kfcampbell commented Jun 5, 2023

gr2m commented Jun 5, 2023

wolfy1339 commented Jun 5, 2023

wolfy1339 commented Jun 5, 2023

kfcampbell commented Jun 6, 2023

kfcampbell commented Jun 6, 2023 •

edited

Loading

kfcampbell commented Jun 6, 2023

wolfy1339 commented Jun 6, 2023

kfcampbell commented Jun 6, 2023

wolfy1339 commented Jun 6, 2023

kfcampbell commented Jun 6, 2023

wolfy1339 left a comment

wolfy1339 Jun 6, 2023

wolfy1339 Jun 6, 2023

kfcampbell Jun 6, 2023

kfcampbell commented Jun 6, 2023

kfcampbell commented Jun 6, 2023

github-actions bot commented Jun 6, 2023

fix(build): switch to esbuild and tsc from pika #345

fix(build): switch to esbuild and tsc from pika #345

Conversation

kfcampbell commented Jun 5, 2023

Behavior

Before the change?

After the change?

Additional info

Pull request checklist

Does this introduce a breaking change?

Pull request type

nickfloyd left a comment

Choose a reason for hiding this comment

wolfy1339 commented Jun 5, 2023

gr2m commented Jun 5, 2023

kfcampbell commented Jun 5, 2023

gr2m commented Jun 5, 2023

wolfy1339 commented Jun 5, 2023

kfcampbell commented Jun 5, 2023

gr2m commented Jun 5, 2023

wolfy1339 commented Jun 5, 2023

wolfy1339 commented Jun 5, 2023

kfcampbell commented Jun 6, 2023

kfcampbell commented Jun 6, 2023 • edited Loading

kfcampbell commented Jun 6, 2023

wolfy1339 commented Jun 6, 2023

kfcampbell commented Jun 6, 2023

wolfy1339 commented Jun 6, 2023

kfcampbell commented Jun 6, 2023

wolfy1339 left a comment

Choose a reason for hiding this comment

wolfy1339 Jun 6, 2023

Choose a reason for hiding this comment

wolfy1339 Jun 6, 2023

Choose a reason for hiding this comment

kfcampbell Jun 6, 2023

Choose a reason for hiding this comment

kfcampbell commented Jun 6, 2023

kfcampbell commented Jun 6, 2023

github-actions bot commented Jun 6, 2023

kfcampbell commented Jun 6, 2023 •

edited

Loading