verify npm package #77

le-cong · 2024-04-17T02:59:49Z

Fixes #75

le-cong · 2024-04-17T17:31:18Z

.github/workflows/ci.yml

        run: npm run ci:lint
      - name: Run Tests
        run: npm run ci:test
+        env:


needed to pass tests which access npm

le-cong · 2024-04-17T19:42:30Z

introducing a new step in publish-beta.yml (between Publish Beta and Comment npm publish result), e.g. smartdata test

carlansley · 2024-04-17T20:35:39Z

src/validate-npm-package/validate-npm-package.ts

+async function obtainNpmPackage(packageVersion: string, workFolder: string): Promise<void> {
+  await addNPMRCFile(workFolder);
+
+  const fullCommandLine = `npm pack ${packageVersion}`;


could we do an npm install somewhere instead? then we could do an import() and make sure all the machinery works (including the package.json is ok)

carlansley · 2024-04-17T20:35:58Z

package.json

 {
  "name": "@checkdigit/github-actions",
-  "version": "2.0.0",
+  "version": "2.0.1",


since we're adding a new action, think this should be a MINOR

carlansley · 2024-04-17T20:36:56Z

src/validate-npm-package/validate-npm-package.ts

+async function verifyImportEntryPoints(packageFolder: string): Promise<void> {
+  const packageJson = JSON.parse(await fs.readFile(`${packageFolder}/package.json`, 'utf8')) as PackageJson;
+  const isEsm = packageJson.type === 'module';
+  const bundleFolder = isEsm ? 'dist-mjs' : 'dist';


if npm install isn't an option, think it would be better to simulate what node does, i.e. read the package.json and figure out the entry point from that.

I think npm install and then doing what deployer does (import and then ping operation) would be good.

this is for any package (not necessarily a service)? I was thinking an npm install + importing it from some generated typescript. then compile & run. that way you know the types are ok and the entry point is correct.

for services, the "import+compile" way might not be able to verify their api endpoints which are not exported. many of their root index.ts only export swagger.ts, like:

// index.ts export * as v1 from './api/v1/swagger';

so even if there is an issue, it might not be exposed.

in the new-new world a service is started by awaiting the default export of the main entry point. so this would expose an issue with API/service code once that starts rolling out. also, I feel the primary job of this action is to validate the npm package itself, which is effectively whatever is exported from the root index.ts, vs functionality as a service. we have other things that do that.

that makes total sense

this is probably a known limitation using import: if the package references excluded(e.g. from *.test.ts) types or values but doesn't export them through its root index.ts, then no error will be reported.
on the other hand, using node cli to load that package's index.ts can expose the problem though.

le-cong · 2024-04-18T23:56:44Z

as part of the latest commit, i've updated tests quite a bit mainly because:

the check-label tests are tangled with the current version, which makes the test tricky and flaky;
fixed other flaky tests dealing with temporary folders
refactored github nock to make the usage of different PRs easier to understand

…ror message in CI is not confusing

adcreare

LGTM - very comprehensive

carlansley · 2024-04-29T15:55:43Z

.github/workflows/ci.yml

this can be updated to use 20.x/22.x now

carlansley · 2024-04-29T15:58:59Z

src/validate-npm-package/index.ts

+
+import main from './validate-npm-package';
+
+main(getInput('betaPackage'))


i forget what issues there might be with this with actions, but can we not use top-level await here? its being compiled to an .mjs file, figure it should Just Work?

carlansley · 2024-04-30T17:07:14Z

src/comment-npm-publish/index.ts

-    // eslint-disable-next-line unicorn/no-process-exit
-    process.exit(1);
-  });
+await main();


given we're just awaiting main() at the top level of the module, do we need the main function at all, just make everything top level? this feels very script-y anyway

tried that and making everything top level, it works but there is one advantage in having a 'main' function when it comes to testing.
what happens is that we'll have to do dynamic import in the tests; and before the dynamic import we normally setup nocking and environment variables. Jest has its own module catching mechanism, which causes some of the tests start failing. one trick is to do jest.resetModules() everywhere.
it seems easier and less surprise if we call the main function instead.
what do you think? should we make the change, or keep the main?

if we're await()ing main, isn't it effectively having everything top-level anyway? my thought is we move main() into a separate module, and all the index.ts does is import and await it. that way tests can do their thing without side effects going on at the module level.

carlansley · 2024-05-14T18:06:25Z

src/comment-npm-publish/index.ts

-    // eslint-disable-next-line unicorn/no-process-exit
-    process.exit(1);
-  });
+await main();


if we're await()ing main, isn't it effectively having everything top-level anyway? my thought is we move main() into a separate module, and all the index.ts does is import and await it. that way tests can do their thing without side effects going on at the module level.

github-actions · 2024-06-04T22:13:48Z

Coverage after merging validate-npm-package into main will be

58.72%	▴ +4.17%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
src/check-imports
check-imports.ts	0%	0%	0%	0%	1, 1, 10–19, 2, 20–29, 3, 30–38, 4–9
index.ts	0%	0%	0%	0%	1, 1–5
package-lock-file-util.ts	100%	100%	100%	100%
packages-not-allowed.ts	100%	100%	100%	100%
src/check-label
check-label.ts	94.23%	89.47%	100%	95.12%	62–64, 71–73
index.ts	0%	0%	0%	0%	1, 1–5
src/check-pr-reviews
check-pr-reviews.ts	0%	0%	0%	0%	1, 1, 10–19, 2, 20–29, 3, 30–39, 4, 40–49, 5, 50, 6–9
index.ts	0%	0%	0%	0%	1, 1–5
src/check-published
check-published.ts	0%	0%	0%	0%	1, 1, 10–19, 2, 20–29, 3, 30–39, 4, 40–49, 5, 50–59, 6, 60–65, 7–9
index.ts	0%	0%	0%	0%	1, 1–5
slack.ts	0%	0%	0%	0%	1, 1, 10–19, 2, 20–29, 3, 30–39, 4, 40–49, 5, 50–59, 6–9
src/comment-npm-publish
comment-npm-publish.ts	0%	0%	0%	0%	1, 1, 10–19, 2, 20–22, 3–9
index.ts	0%	0%	0%	0%	1, 1–5
src/coverage-reporter
cli.ts	0%	0%	0%	0%	1, 1, 10–19, 2, 20–29, 3, 30–39, 4, 40–49, 5, 50, 6–9
comment.ts	0%	0%	0%	0%	1, 1, 10–19, 2, 20–29, 3, 30–39, 4, 40–49, 5, 50–52, 6–9
coverage-reporter.ts	0%	0%	0%	0%	1, 1, 10, 100–109, 11–19, 2, 20–29, 3, 30–39, 4, 40–49, 5, 50–59, 6, 60–69, 7, 70–79, 8, 80–89, 9, 90–99
delete-old-comments.ts	0%	0%	0%	0%	1, 1, 10–19, 2, 20–29, 3, 30–39, 4, 40–49, 5, 50–59, 6, 60–69, 7, 70–72, 8–9
get-changes.ts	0%	0%	0%	0%	1, 1, 10–19, 2, 20–29, 3, 30–39, 4, 40–41, 5–9
html.ts	100%	100%	100%	100%
index.ts	0%	0%	0%	0%	1, 1–5
lcov.ts	93.97%	90%	100%	94.32%	122–132, 139
options.ts	0%	0%	0%	0%	1, 1, 10–14, 2–9
tabulate.ts	99.56%	97.96%	100%	100%	159
util.ts	100%	100%	100%	100%
src/github-api
index.ts	80.42%	68.42%	100%	82.42%	109–112, 134–137, 156–165, 180–183, 187–190, 203–206, 210–213, 224–227, 238–240, 246–248, 250–253, 49, 55, 57–61, 70–73, 77–80, 95–97
src/nocks
github.test.ts	98.95%	90.91%	100%	99.44%	164, 64
src/perform-bundle
analyze.ts	0%	0%	0%	0%	1, 1, 10–19, 2, 20–29, 3, 30–36, 4–9
deployer.ts	0%	0%	0%	0%	1, 1, 10–18, 2–9
index.ts	0%	0%	0%	0%	1, 1–5
perform-bundle.ts	0%	0%	0%	0%	1, 1, 10–19, 2, 20–29, 3, 30–39, 4, 40–42, 5–9
src/prepare-beta
index.ts	0%	0%	0%	0%	1, 1–5
package.ts	93.33%	66.67%	100%	95%	22–24
prepare-beta.ts	0%	0%	0%	0%	1, 1, 10–17, 2–9
src/publish-beta
compile.ts	0%	0%	0%	0%	1, 1, 10–14, 2–9
files.ts	92.86%	91.67%	100%	92.86%	28–31
index.ts	0%	0%	0%	0%	1, 1–5
package.ts	94.92%	83.33%	100%	96%	22–24
publish-beta.ts	0%	0%	0%	0%	1, 1, 10–19, 2, 20–28, 3–9
publish.ts	82.35%	100%	50%	82.76%	25–29
src/validate-npm-package
index.ts	0%	0%	0%	0%	1, 1–5
validate-npm-package.ts	100%	100%	100%	100%

carlansley

lftm, the only issue i can think of is would this work with packages that export weird things (e.g. *-config repos). e.g. jest-config doesn't export anything at all (maybe it should?). i suppose we can cross that bridge when we come to it.

github-actions · 2024-06-05T17:20:24Z

✅ PR review status - All reviews completed and approved!

main function

e8c3fc7

le-cong added the PATCH label Apr 17, 2024

le-cong self-assigned this Apr 17, 2024

le-cong added 15 commits April 17, 2024 00:13

handle npmrc

26f4f81

move npmrc handling to the first step

8cd39cc

pass npm auth token

9a1e1bb

remove npmrc

dcc4834

remove unused import

e26e8d2

add back NPM_TOKEN

f8e16d1

use NPM_TOKEN_PUBLISH instead

2c7d9dd

put back npmrc

6e8e799

NODE_AUTH_TOKEN

f28a268

xxx

71fb7ea

1

2f66088

2

5036f83

add npmrc to package folder

b32c182

fix coverage, refactor

53247cf

npmrc

35707eb

le-cong commented Apr 17, 2024

View reviewed changes

le-cong added 2 commits April 17, 2024 14:26

cleanup

4904446

update dependencies

968c27c

le-cong requested review from adcreare and carlansley April 17, 2024 19:37

carlansley requested changes Apr 17, 2024

View reviewed changes

le-cong added 2 commits April 18, 2024 15:29

use npm import approach

9f7b39e

bump MINOR version, fix flaky tests, refactor tests

63dc2ae

le-cong requested a review from carlansley April 18, 2024 23:56

swap expected/actual values in the assertion so that the displayed er…

736a28c

…ror message in CI is not confusing

adcreare approved these changes Apr 22, 2024

View reviewed changes

carlansley requested changes Apr 29, 2024

View reviewed changes

address carl's comments

f083b5a

le-cong requested a review from carlansley April 29, 2024 20:17

carlansley requested changes Apr 30, 2024

View reviewed changes

le-cong requested a review from carlansley April 30, 2024 21:13

carlansley requested changes May 14, 2024

View reviewed changes

le-cong added 2 commits June 4, 2024 10:32

update dependencies

dfacc90

refactoring

9b1a9dd

le-cong requested a review from carlansley June 4, 2024 22:15

carlansley approved these changes Jun 5, 2024

View reviewed changes

le-cong merged commit 6c6faab into main Jun 5, 2024

le-cong deleted the validate-npm-package branch June 5, 2024 17:24


		import main from './validate-npm-package';

		main(getInput('betaPackage'))

verify npm package #77

verify npm package #77

Uh oh!

Conversation

le-cong commented Apr 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

le-cong commented Apr 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

le-cong commented Apr 18, 2024

Uh oh!

adcreare left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 4, 2024

Uh oh!

carlansley left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants