Make sure CI builds test latest versions of dependencies

[cjbarth]

This will prevent the CI builds from relying on package-lock.json. The only question that I have is about including package-lock.json in commits. I think we should keep it so that we can rebuilt exact artifacts at tags, but, OTOH, we might want to avoid using it because it would mask stuff that appears when doing development. I favored the rebuilding artifacts argument in this PR, but am open to discussion.

[cjbarth]

@forty Would you like to have a look at this based on our discussion in #566 ?

[forty]

@cjbarth That looks good to me 👍

[forty]

Her, actually, 2 comments:

• I noticed that you reverted the removal of the package-lock, but I think npm install will actually follow the package-lock if it's there
• npm install runs the prepare script, which causes tsc to be invoked twice (since npm run test also invokes it explicitely) - no harm, except slower CI

[cjbarth]

Basically, we have three choices:

1. Use npm-shrinkwrap.json, which will lock packages for developers, CI, and consumers
2. Use package-lock.json, which will lock packages for developers and CI, but not for consumers
3. Use neither, which will mean that each installation, even for developers and CI builds will result in a different behavior

If we use 3, then we can't get reproducible builds from a tag. I'm not sure if that is what we're after.

[forty]

One option would be to remove the file from Git and for the CI to publish the generated package-lock as an artifact, so we could always go back to a given build and reproduce it. We could also continue shipping it in the published package, even if it's ignored, but just as a kind of documentation.

Note that I don't know GH CI at all, so I don't really know what's possible/convenient.

[gugu]

I think it is better not to remove it. We can add one more test npm ci && npm update to test if the module runs on the latest version of dependencies

[forty]

That works too indeed, but note that you would need something like npm update --depth 99 to update sub dependencies.

Another option is to do the extra test with rm package-lock.json && npm install

[markstos]

I'm in favor of keeping a lock file for consistent and reproducible results everywhere.

I think I'm most aligned with the shrinkwrap option. I use Yarn and yarn.lock in my own projects and do commit and version the file.

[cjbarth]

It appears that we need to do something. The lockfile being the way it is allows for the tests to run and pass. However, if I delete package-lock.json and delete node_modules and then npm install. The tests fail with the following:

TypeError: lib/passport-saml/index.js: Emit skipped
    at getOutput (/home/cjbarth/code/passport-saml/node_modules/ts-node/src/index.ts:755:17)
    at Object.compile (/home/cjbarth/code/passport-saml/node_modules/ts-node/src/index.ts:968:32)
    at Module.m._compile (/home/cjbarth/code/passport-saml/node_modules/ts-node/src/index.ts:1056:42)
    at Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Object.require.extensions.(anonymous function) [as .js] (/home/cjbarth/code/passport-saml/node_modules/ts-node/src/index.ts:1059:12)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
    at Function.Module._load (internal/modules/cjs/loader.js:585:3)
    at Module.require (internal/modules/cjs/loader.js:692:17)
    at require (internal/modules/cjs/helpers.js:25:18)
    at Object.<anonymous> (/home/cjbarth/code/passport-saml/test/test-signatures.spec.ts:1:1)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Module.m._compile (/home/cjbarth/code/passport-saml/node_modules/ts-node/src/index.ts:1056:23)
    at Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Object.require.extensions.(anonymous function) [as .ts] (/home/cjbarth/code/passport-saml/node_modules/ts-node/src/index.ts:1059:12)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
    at Function.Module._load (internal/modules/cjs/loader.js:585:3)
    at Module.require (internal/modules/cjs/loader.js:692:17)
    at require (internal/modules/cjs/helpers.js:25:18)
    at Object.exports.requireOrImport (/home/cjbarth/code/passport-saml/node_modules/mocha/lib/esm-utils.js:42:12)
    at Object.exports.loadFilesAsync (/home/cjbarth/code/passport-saml/node_modules/mocha/lib/esm-utils.js:55:34)
    at process._tickCallback (internal/process/next_tick.js:68:7)
    at Function.Module.runMain (internal/modules/cjs/loader.js:834:11)
    at startup (internal/bootstrap/node.js:283:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:623:3)

[forty]

@markstos My understanding is that yarn.lock works similarly to npm's package-lock.json and is ignored for non "top level" package (so the argument of making sure the tests passes without still holds)

I think shrinkwrap is more or less deprecated, basically since yarn was launched, as their main "innovation" at the time was precisely to ignore shrinkwraps files for libraries if I remember correctly.

[forty]

@cjbarth It was just a tiny problem https://github.com/node-saml/passport-saml/pull/572/files

It works with recent packages after that

[markstos]

@forty That sounds right.

I used shrinkwrap files years ago and there were many problems which is why we switched to Yarn.

[cjbarth]

Ok, things look better now. @markstos @forty , did you want to review this?

[forty]

Looks good to me 👍