Skip to content

Relax our dependency criteria #1503

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cristiano-belloni merged 6 commits into from
Mar 25, 2022
Merged

Relax our dependency criteria #1503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d5b238b
use devDependencies and only error on screen for dependencies that ar…
Mar 24, 2022
eb03a37
Better messaging
Mar 24, 2022
5732dff
Better wording
Mar 24, 2022
b03d920
simplify
Mar 24, 2022
b88702a
prevent clashes
Mar 24, 2022
a1a2e8d
Create twenty-clouds-invite.md
cristiano-belloni Mar 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/twenty-clouds-invite.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"modular-scripts": patch
---

Relax our dependency discovery criteria to include dev dependencies and just warn when a dependency is not found.
37 changes: 19 additions & 18 deletions packages/modular-scripts/src/utils/getPackageDependencies.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import * as path from 'path';
import * as fs from 'fs-extra';
import { Project } from 'ts-morph';
import type { CoreProperties } from '@schemastore/package';
import type { CoreProperties, Dependency } from '@schemastore/package';
import getModularRoot from './getModularRoot';
import getLocation from './getLocation';
import getWorkspaceInfo from './getWorkspaceInfo';
import * as logger from './logger';

type DependencyManifest = NonNullable<CoreProperties['dependencies']>;

Expand Down Expand Up @@ -46,19 +47,21 @@ export async function getPackageDependencies(
const targetLocation = await getLocation(target);
const workspaceInfo = getWorkspaceInfo();

const rootPackageJsonDependencies =
(
fs.readJSONSync(
path.join(getModularRoot(), 'package.json'),
) as CoreProperties
).dependencies || {};
const rootManifest = fs.readJSONSync(
path.join(getModularRoot(), 'package.json'),
) as CoreProperties;

const targetPackageJsonDependencies =
(
fs.readJSONSync(
path.join(targetLocation, 'package.json'),
) as CoreProperties
).dependencies || {};
const targetManifest = fs.readJSONSync(
path.join(targetLocation, 'package.json'),
) as CoreProperties;

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

/* Get regular dependencies from package.json (regular) or root package.json (hoisted)
* Exclude workspace dependencies. Error if a dependency is imported in the source code
Expand All @@ -67,12 +70,10 @@ export async function getPackageDependencies(
const manifest = getDependenciesFromSource(targetLocation)
.filter((depName) => !(depName in workspaceInfo))
.reduce<DependencyManifest>((manifest, depName) => {
const depVersion =
targetPackageJsonDependencies[depName] ??
rootPackageJsonDependencies[depName];
const depVersion = deps[depName];
Copy link
Copy Markdown
Contributor

@steveukx steveukx Mar 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@cristiano-belloni cristiano-belloni Mar 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (!depVersion) {
throw new Error(
`Package ${depName} imported in ${target} source but not found in package dependencies or hoisted dependencies`,
logger.error(
`Package ${depName} imported in ${target} source but not found in package dependencies or hoisted dependencies - this will prevent you from successfully build, start or move packages in the next release of modular`,
);
}
manifest[depName] = depVersion;
Expand Down