feat(env-options): env-options as separate importable package

[erights]

Lets us do the many // TODO Use environment-options.js, rather than hand rolling process.env[name] handling each time.

[gibson042]

Aren't there more files that should make use of this?

[erights]

Aren't there more files that should make use of this?

Done: track-turns.js and exo-makers.js

Also virtualObjectManager.js, but that's in agoric-sdk so will happen separately.

@michaelfig , please let me know if you'd like me to back out the changes to eventual-send because you want it to remain dependency-free. LOW URGENCY.

[gibson042]

One small code organizational question, but LGTM no matter how it's resolved.

[erights]

Reviewers, at https://github.com/endojs/endo/actions/runs/5034078931/jobs/9028682745?pr=1585 I get

npm ERR! code E404
npm ERR! 404 Not Found - GET https://registry.npmjs.org/@endo%2fenv-options - Not found
npm ERR! 404 
npm ERR! 404  '@endo/env-options@^0.1.0' is not in the npm registry.
npm ERR! 404 You should bug the author to publish it (or use the name yourself!)
npm ERR! 404 It was specified as a dependency of 'ses'
npm ERR! 404 
npm ERR! 404 Note that you can also install from a
npm ERR! 404 tarball, folder, http url, or git url.

This prevents me from merging. What should I do about this?

[gibson042]

I am able to reproduce the issue locally with Node.js v14. Basically, the script tries to npm pack the ses package and then install the resulting tarball, which works on master (where the ses package has no dependencies) but not on this PR (where the ses package has a dependency on @endo/env-options@^0.1.0, which does not exist in the npm registry because this PR just created it. It would probably be fixed by introducing the package in one PR and updating the ses package to use it in a second PR, but would recur with every relevant change to the env-options package. In some ways, the ses-integration-test package seems like an indirect and incomplete test that the ses package has no dependencies, which impacts efforts such as this to share robust code across ses and other Endo packages.

I think we should keep the ses package free of dependencies, and either tolerate duplication of env-options code (cf. isTypedArray in both ses and pass-style) or keep the code in ses for deep import from other packages (as it seems may have previously been the case for ses/lockdown?).

Tagging @kriskowal for an opinion.

[kriskowal]

I think in this case, the test might be holding us back. @michaelfig Do you recall what kind of regression the SES integration test is protecting us from, since it appears to test the published sources and not what is in the working copy? Is there a possibility that the test is misbehaving only because it uses npm instead of yarn (Was this intended to verify that npm continued to work even tho we’re using yarn?)

[michaelfig]

I think in this case, the test might be holding us back.

I think in this case the test is revealing that the current PR is making a breaking change that may affect our downstream users. We should not take that lightly.

@michaelfig Do you recall what kind of regression the SES integration test is protecting us from,

From packages/ses-integration-test/README.md:

This test folder creates a tarball of the main repository and runs the main tests in a browser context with multiple bundlers and tools. The goal is to ensure that someone downstream of the package will not have an issue when importing it.

since it appears to test the published sources and not what is in the working copy?

It certainly is testing what is in the working copy. I agree with @gibson042's assessment: it's only attempting to download an unpublished package because ses/package.json takes a hard dependency on env-options. Besides Richard's suggested workarounds we could do one of:

• maybe make the env-options dep into a peerDependency
• npm pack the env-options package during the ses-integration-test and use it just like we use the ses pack

Is there a possibility that the test is misbehaving only because it uses npm instead of yarn (Was this intended to verify that npm continued to work even tho we’re using yarn?)

That's a red herring. Yarn's behaviour does not differ in this regard.

[erights]

I'm going with

It would probably be fixed by introducing the package in one PR and updating the ses package to use it in a second PR, but

at least as far as this first PR, where I have reverted all changes to the ses package. Will open a second PR with only these postponed changes where we can discuss what to do with them.