Allow set.env plugin to return an empty object

[lpsinger]

Thank you for helping out! ✨

We really appreciate your commitment to improving Architect

To maintain a high standard of quality in our releases, before merging every pull request we ask that you've completed the following:

• Forked the repo and created your branch from main
• Made sure tests pass (run npm it from the repo root)
• Expanded test coverage related to your changes:
  • Added and/or updated unit tests (if appropriate)
  • Added and/or updated integration tests (if appropriate)
• Updated relevant documentation:
  • Internal to this repo (e.g. readme.md, help docs, inline docs & comments, etc.)
  • Architect docs (arc.codes)
• Summarized your changes in changelog.md
• Linked to any related issues, PRs, etc. below that may relate to, consume, or necessitate these changes

Please also be sure to completed the CLA (if you haven't already).

Learn more about contributing to Architect here.

Thanks again!

[ryanblock]

Ok, but why?

[lpsinger]

Ok, but why?

Because I have authored a plugin whose set.env entry point must return some environment variables or none depending on the configuration.

[ryanblock]

These semantics are deliberate and will impact consumers of this module downstream – in other words, this PR is a breaking change. A far simpler solution might be to return Object.keys(yourEnv).length ? yourEnv : undefined.

[lpsinger]

A far simpler solution might be to return Object.keys(yourEnv).length ? yourEnv : undefined.

Do you mean, in my plugin? That would still require a code change in @architect/inventory because the return value undefined would fail the check !is.object(result).

[ryanblock]

Got it, so the feature request is to enable set.env to not return; we can look at that, although we'd need to analyze the impact on sandbox, package, and any other consumers to ensure this wouldn't be a breaking change. Have you done that yet, by chance?

[lpsinger]

Got it, so the feature request is to enable set.env to not return;

It would meet our needs if Architect allowed a return value from set.env of either undefined or {}.

we can look at that, although we'd need to analyze the impact on sandbox, package, and any other consumers to ensure this wouldn't be a breaking change. Have you done that yet, by chance?

No, but I don't think that this is a breaking change. It doesn't alter the API, and I don't see how any plugin in the wild could already be depending upon undefined or {} being invalid return values here.

[ryanblock]

Returning an empty object would be a breaking change if consumers of the env var property assume it has always properties, which it currently would. Returning undefined is probably fine, but I haven't looked at the impact, and likely won't have time to do that in the immediate future – so if you'd like this PR to go through, will need a perspective on that. Thank you!

[lpsinger]

Returning an empty object would be a breaking change if consumers of the env var property assume it has always properties, which it currently would.

Isn't Architect itself the only consumer of the return value of the plugin entry points?

[ryanblock]

Everything in Architect is published as a module, so we make the assumption that developers may use them outside CLI workflows. But perhaps more importantly: it's reasonable to assume people will write tests around their Arc plugins.