Relax our dependency criteria

[cristiano-belloni]

• Allow devDependencies to be in the dependency manifest
• Prevent throwing when generating the manifest if there are transitive deps, show a warning that this will be an error in next major

[changeset-bot]

🦋 Changeset detected

Latest commit: a1a2e8d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
modular-scripts	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[coveralls]

Coverage increased (+0.05%) to 28.742% when pulling a1a2e8d on feature/relax-get-dependencies into 178796b on main.

[steveukx]

nit: as an option for making this a bit less verbose:

const deps = Object.assign(Object.create(null).
  targetManifest.devDependencies,
  rootManifest.devDependencies, 
  targetManifest.dependencies, 
  rootManifest.dependencies, 
);

// ...
manifest[depName] = deps[depName];
if (manifest[depName] === undefined) {
  logger.error(...);
}

[cristiano-belloni]

Done with

const deps = {
    ...rootManifest.dependencies,
    ...targetManifest.dependencies,
    ...rootManifest.devDependencies,
    ...targetManifest.devDependencies,
  };

[steveukx]

Are there any tests of the build command that could cover this change?

[cristiano-belloni]

Are there any tests of the build command that could cover this change?

We have snapshot tests for the dependency manifest, which pass without modifications because we just relaxed our criteria (so there's no change). We don't have any test of the build command failing because of some deps not in package.json (but we wouldn't need them now, since with this PR it would never fail).

[steveukx]

This is a potential prototype pollution - if the depName happens to be toString then it will be a property of deps whether it's specified or not.

[cristiano-belloni]

Agreed: using Object.assign(Object.create(null), ...) with a type assertion