Native ESM #57

smorimoto · 2024-01-09T12:40:35Z

Context

When building actions in ESM, some incompatibilities were caused by packaging using ncc, which caused runtime errors. So the solution is to switch the source to native ESM and generate code for ESM and CJS at build time.

Changes:

tsup

The build system is changed to tsup, and tsc is now used only for type checking. This makes support for both esm and cjs much simpler.

Vitest

Switch to vitest for faster and easier testing in ESM.

Biome

This project is not very complex, so formatting and linting are switched to biome. This results in faster, simpler setup and less dependencies.

`.npmignore`

If the files field is already included in package.json, you should not have to add the .npmignore file. As you can see by running dry-run, there are much fewer sources in the package now.

~/src/github.com/github/dependency-submission-toolkit native-esm
❯ npm pack --dry-run
npm notice 
npm notice 📦  @github/dependency-submission-toolkit@1.2.9
npm notice === Tarball Contents === 
npm notice 1.1kB  LICENSE           
npm notice 2.8kB  README.md         
npm notice 7.6kB  dist/index.cjs    
npm notice 29.0kB dist/index.cjs.map
npm notice 11.1kB dist/index.d.cts  
npm notice 11.1kB dist/index.d.ts   
npm notice 6.9kB  dist/index.js     
npm notice 29.0kB dist/index.js.map 
npm notice 1.6kB  package.json      
npm notice === Tarball Details === 
npm notice name:          @github/dependency-submission-toolkit         
npm notice version:       1.2.9                                         
npm notice filename:      github-dependency-submission-toolkit-1.2.9.tgz
npm notice package size:  17.1 kB                                       
npm notice unpacked size: 100.2 kB                                      
npm notice shasum:        aa2ad1f5115dd6020565878219052266e8c7350c      
npm notice integrity:     sha512-Iphxp7mGIctTS[...]soLs4oK6rz76Q==      
npm notice total files:   9                                             
npm notice 
github-dependency-submission-toolkit-1.2.9.tgz

Unpacked Size

1.16 MB -> 0.1 MB

smorimoto · 2024-01-09T21:44:12Z

Ready to go!

febuiles · 2024-01-10T07:28:00Z

@smorimoto thank you very much for this contribution there's, a lot we can benefit from in here.

Can I ask you to split this into different PRs? From going over the code once I can think of a few:

Upgrade Node to 20.
Add ESM-required changes.
Add extra tooling (e.g. Biome).

This can help us review and more quickly address each change. Thanks again.

smorimoto · 2024-01-11T10:23:09Z

@febuiles Thanks a lot for the quick response! Actually, I considered that at first, but it's a bit hard to split things into different PRs except Node.js 20 upgrade.

The reasons for each are:

tsup

This is required to deal with the NCC outputting code which is not ESM compliant.

Vitest

This is not strictly required, but you will need to make some configuration changes to Jest to test the ESM code with Jest.

Biome

This is also not strictly required, but the difference is much smaller than changing ESLint configuration for ESM.

smorimoto · 2024-01-12T00:33:01Z

tsup.config.ts

+  minify: true,
+  sourcemap: true,
+  splitting: true,
+  target: 'node16',


I think Node.js 16 is the lower bound in the current "works but deprecated" sense. You should be able to easily update the lower bound to Node.js 20 by simply changing this in the future.

smorimoto · 2024-01-12T00:34:03Z

tsconfig.json

Just use the strict recommended TSConfig.

smorimoto · 2024-01-12T00:34:41Z

example/tsconfig.json

Just use the strict recommended TSConfig.

smorimoto · 2024-01-12T00:45:46Z

I tried to reduce the diff as much as possible. This makes reviewing easier (I guess).

smorimoto · 2024-01-12T00:47:12Z

.github/workflows/dependency-submission-example.yml

      - name: Install NPM dependencies
        run: npm ci
      - name: Run all NPM build/test actions
-        run: npm -w:example rebuild && npm run all -w:example


Using prepare instead of prepack eliminates the need for rebuild. But prepack may be less noisy during the development process.

smorimoto · 2024-01-12T00:47:25Z

.github/workflows/test.yml

      - name: Run all NPM build/test actions
-        run: npm rebuild && npm run all


smorimoto · 2024-01-12T00:48:01Z

package.json

+    "ci": "biome ci .",
+    "format": "biome format --write .",
+    "lint": "biome lint --apply .",
+    "prepare": "npm run build",


You can choose prepare or prepack depending on your needs.

smorimoto · 2024-01-12T00:49:04Z

.github/workflows/publish.yml

        with:
-          node-version: 16.x
-          registry-url: https://registry.npmjs.org/
-          cache: npm


For small projects, the cache will probably just cause thrashing, which is just a waste of compute resources.

smorimoto · 2024-01-12T00:53:11Z

example/package.json

I just removed unnecessary things out and ran sort-package-json to organise package.json order.

smorimoto · 2024-01-12T00:53:34Z

package.json

Ran sort-package-json to organise package.json order.

smorimoto · 2024-01-12T00:54:36Z

package.json

-  "directories": {
-    "test": "test"
-  },


Should be unnecessary.

smorimoto · 2024-01-12T00:54:58Z

package.json

@@ -2,67 +2,54 @@
  "name": "@github/dependency-submission-toolkit",
  "version": "1.2.9",
  "description": "A TypeScript library for creating dependency snapshots.",
-  "prepare": "npm run build",


Probably misplaced at the top level?

smorimoto · 2024-01-12T01:00:20Z

.npmignore

Conflicts with files in package.json and is not needed: https://docs.npmjs.com/cli/v10/configuring-npm/package-json#files

smorimoto · 2024-01-12T01:01:25Z

@febuiles I left some comments. I hope these help the review process.

febuiles · 2024-01-12T08:38:57Z

I think all of this makes sense, thanks for your contribution. I went over it and things look good, I'll try to carve up some time to do local testing before merging by next week at the latest.

smorimoto · 2024-01-12T12:41:48Z

@febuiles Sorry for the lot of diff 😅
As a side note, the results of the test run for the example action can also be found here: https://github.com/smorimoto/dependency-submission-toolkit/actions/runs/7502291922

smorimoto · 2024-01-12T12:44:59Z

.github/workflows/dependency-submission-example.yml

      - name: Install NPM dependencies
        run: npm ci
      - name: Run all NPM build/test actions
-        run: npm -w:example rebuild && npm run all -w:example
+        run: npm run --workspace example all


The old way doesn't seem to work, so for clarity, use the verbose flag here. That said, the short hand should work if you do the following: -w example or -w=example, not -w:example

Signed-off-by: Sora Morimoto <sora@morimoto.io>

smorimoto · 2024-01-12T12:50:28Z

example/npm-detector.ts

-    ['list', '--prod', '--all', '--json'],
+    ['list', '--all', '--json', '--omit', 'dev'],


--prod is already deprecated

hmaurer · 2024-01-12T14:14:38Z

example/tsconfig.json

-    "*.test.ts"
-  ]
+  "exclude": ["dist", "node_modules"],
+  "extends": "@tsconfig/strictest/tsconfig.json"


TIL, this is neat!

I always recommend using this!

febuiles · 2024-01-12T14:51:50Z

@smorimoto Thank you very much for your contribution. Once #59 is merged we'll do a new major release that will include these changes.

smorimoto · 2024-01-12T14:53:27Z

@febuiles My pleasure! Thank you so much for this fantastic package! The OCaml community is really actively taking advantage of this 🙂

smorimoto · 2024-01-12T15:02:09Z

Confirmed v2 works with the ESM GHA! Thanks a lot!

smorimoto · 2024-01-12T15:19:18Z

@febuiles @hmaurer Offtopic: In the OCaml community, the OCaml core team has already released a stable version of the dependency submission action, and we want to make it well-integrated with GitHub, but is there a way to do this? I couldn't find that anywhere.

febuiles · 2024-01-12T15:40:28Z

@smorimoto I think @jonjanego could help with that!

jonjanego · 2024-01-12T15:52:58Z

@smorimoto Thanks for reaching out. For purposes of keeping this PR tidy, would you mind sharing some more details about what the OCaml community has built, and what sort of integration that you're thinking of, in a new issue in this repo so we can discuss it there?

febuiles · 2024-01-17T05:22:43Z

@smorimoto the ESM changes are causing at least one issue I'm aware of: https://github.com/advanced-security/maven-dependency-submission-action/actions/runs/7505596002/job/20435167951, did we miss any steps here?

smorimoto · 2024-01-17T19:13:53Z

@febuiles The reason for that seems to be that the index.ts does not export DependencyScope.

dependency-submission-toolkit/src/index.ts

Line 1 in 86a613f

import { BuildTarget, Manifest } from './manifest.js'

How about exposing all public APIs in the package root like this?

export * from './manifest.js'
export * from './package-cache.js'
export * from './package.js'
export * from './snapshot.js'

smorimoto · 2024-01-17T20:29:56Z

@febuiles See this PR! #63

smorimoto requested a review from a team as a code owner January 9, 2024 12:40

smorimoto force-pushed the native-esm branch 4 times, most recently from 8f8a2ee to 34c75ba Compare January 9, 2024 14:08

smorimoto force-pushed the native-esm branch 7 times, most recently from c5146af to e7bd516 Compare January 12, 2024 00:44

smorimoto commented Jan 12, 2024

View reviewed changes

smorimoto force-pushed the native-esm branch from e7bd516 to 34ffc71 Compare January 12, 2024 12:41

smorimoto commented Jan 12, 2024

View reviewed changes

Native ESM

e958f9e

Signed-off-by: Sora Morimoto <sora@morimoto.io>

smorimoto force-pushed the native-esm branch from 34ffc71 to e958f9e Compare January 12, 2024 12:47

smorimoto commented Jan 12, 2024

View reviewed changes

hmaurer reviewed Jan 12, 2024

View reviewed changes

febuiles approved these changes Jan 12, 2024

View reviewed changes

febuiles merged commit 39c8d88 into github:main Jan 12, 2024
3 checks passed

smorimoto deleted the native-esm branch January 12, 2024 14:51

febuiles mentioned this pull request Jan 18, 2024

Bumping version to 2.0.2 #65

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Native ESM #57

Native ESM #57

smorimoto commented Jan 9, 2024 •

edited

Loading

smorimoto commented Jan 9, 2024

febuiles commented Jan 10, 2024

smorimoto commented Jan 11, 2024

smorimoto Jan 12, 2024

smorimoto Jan 12, 2024

smorimoto Jan 12, 2024

smorimoto commented Jan 12, 2024

smorimoto Jan 12, 2024

smorimoto Jan 12, 2024

smorimoto Jan 12, 2024

smorimoto Jan 12, 2024

smorimoto Jan 12, 2024

smorimoto Jan 12, 2024

smorimoto Jan 12, 2024

smorimoto Jan 12, 2024

smorimoto Jan 12, 2024

smorimoto commented Jan 12, 2024

febuiles commented Jan 12, 2024

smorimoto commented Jan 12, 2024

smorimoto Jan 12, 2024

smorimoto Jan 12, 2024

hmaurer Jan 12, 2024

smorimoto Jan 12, 2024

febuiles commented Jan 12, 2024

smorimoto commented Jan 12, 2024

smorimoto commented Jan 12, 2024

smorimoto commented Jan 12, 2024

febuiles commented Jan 12, 2024

jonjanego commented Jan 12, 2024

febuiles commented Jan 17, 2024

smorimoto commented Jan 17, 2024

smorimoto commented Jan 17, 2024

		- name: Run all NPM build/test actions
		run: npm rebuild && npm run all

		['list', '--prod', '--all', '--json'],
		['list', '--all', '--json', '--omit', 'dev'],

Native ESM #57

Native ESM #57

Conversation

smorimoto commented Jan 9, 2024 • edited Loading

Context

Changes:

tsup

Vitest

Biome

.npmignore

Unpacked Size

smorimoto commented Jan 9, 2024

febuiles commented Jan 10, 2024

smorimoto commented Jan 11, 2024

tsup

Vitest

Biome

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smorimoto commented Jan 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smorimoto commented Jan 12, 2024

febuiles commented Jan 12, 2024

smorimoto commented Jan 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

febuiles commented Jan 12, 2024

smorimoto commented Jan 12, 2024

smorimoto commented Jan 12, 2024

smorimoto commented Jan 12, 2024

febuiles commented Jan 12, 2024

jonjanego commented Jan 12, 2024

febuiles commented Jan 17, 2024

smorimoto commented Jan 17, 2024

smorimoto commented Jan 17, 2024

smorimoto commented Jan 9, 2024 •

edited

Loading

`.npmignore`