Skip to content
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.

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

fix(ngcc): do not analyze dependencies for non Angular entry-points #32303

Closed
wants to merge 1 commit into from

Conversation

@JoostK
Copy link
Member

commented Aug 24, 2019

When ngcc is called for a specific entry-point, it has to determine
which dependencies to transitively process. To accomplish this, ngcc
traverses the full import graph of the entry-points it encounters, for
which it uses a dependency host to find all module imports. Since
imports look different in the various bundle formats ngcc supports, a
specific dependency host is used depending on the information provided
in an entry-points package.json file. If there's not enough
information in the package.json file for ngcc to be able to determine
which dependency host to use, ngcc would fail with an error.

If, however, the entry-point is not compiled by Angular, it is not
necessary to process any of its dependencies. None of them can have
been compiled by Angular so ngcc does not need to know about them.
Therefore, this commit changes the behavior to avoid recursing into
dependencies of entry-points that are not compiled by Angular.

In particular, this fixes an issue for packages that have dependencies
on the date-fns package. This package has various secondary
entry-points that have a package.json file only containing a typings
field, without providing additional fields for ngcc to know which
dependency host to use. By not needing a dependency host at all, the
error is avoided.

Fixes #32302

fix(ngcc): do not analyze dependencies for non Angular entry-points
When ngcc is called for a specific entry-point, it has to determine
which dependencies to transitively process. To accomplish this, ngcc
traverses the full import graph of the entry-points it encounters, for
which it uses a dependency host to find all module imports. Since
imports look different in the various bundle formats ngcc supports, a
specific dependency host is used depending on the information provided
in an entry-points `package.json` file. If there's not enough
information in the `package.json` file for ngcc to be able to determine
which dependency host to use, ngcc would fail with an error.

If, however, the entry-point is not compiled by Angular, it is not
necessary to process any of its dependencies. None of them can have
been compiled by Angular so ngcc does not need to know about them.
Therefore, this commit changes the behavior to avoid recursing into
dependencies of entry-points that are not compiled by Angular.

In particular, this fixes an issue for packages that have dependencies
on the `date-fns` package. This package has various secondary
entry-points that have a `package.json` file only containing a `typings`
field, without providing additional fields for ngcc to know which
dependency host to use. By not needing a dependency host at all, the
error is avoided.

Fixes #32302

@JoostK JoostK force-pushed the JoostK:ngcc-non-angular-entrypoints branch from 14799b0 to 8811b6f Aug 24, 2019

@petebacondarwin

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

If, however, the entry-point is not compiled by Angular, it is not
necessary to process any of its dependencies. None of them can have
been compiled by Angular

Is this actually true? Could you not have a package that was not compiled by Angular but somehow exposes an NgModule from a package that was compiled by Angular? I accept that this is probably vanishingly rare but not impossible, right?

@JoostK

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2019

If, however, the entry-point is not compiled by Angular, it is not
necessary to process any of its dependencies. None of them can have
been compiled by Angular

Is this actually true? Could you not have a package that was not compiled by Angular but somehow exposes an NgModule from a package that was compiled by Angular? I accept that this is probably vanishingly rare but not impossible, right?

According to @alxhub, the non-Angular dep would need a metadata.json file to connect the transitive's dep's metadata. More info can be found in Slack.

Also, we are already skipping entry-points that we don't recognize. The difference for date-fns is that it has a typings field, which is enough to create an EntryPoint from, whereas anything without typings field would bail:

const typings = entryPointPackageJson.typings || entryPointPackageJson.types ||
guessTypingsFromPackageJson(fs, entryPointPath, entryPointPackageJson);
if (!typings) {
return null;
}
// An entry-point is assumed to be compiled by Angular if there is either:
// * a `metadata.json` file next to the typings entry-point
// * a custom config for this entry-point
const metadataPath = resolve(entryPointPath, typings.replace(/\.d\.ts$/, '') + '.metadata.json');
const compiledByAngular = entryPointConfig !== undefined || fs.exists(metadataPath);
const entryPointInfo: EntryPoint = {
name: entryPointPackageJson.name,
packageJson: entryPointPackageJson,
package: packagePath,
path: entryPointPath,
typings: resolve(entryPointPath, typings), compiledByAngular,
};
return entryPointInfo;

There even exists a test for this scenario, although the test doesn't verify that no dependency scan is performed at all (as the package being used in the test doesn't have any imports at all):

it('should return an empty array if the target path is not an Angular entry-point', () => {

@alan-agius4

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

While looking at this PR I couldn't help to looking at guessTypingsFromPackageJson

function guessTypingsFromPackageJson(
fs: FileSystem, entryPointPath: AbsoluteFsPath,
entryPointPackageJson: EntryPointPackageJson): AbsoluteFsPath|null {
for (const prop of SUPPORTED_FORMAT_PROPERTIES) {
const field = entryPointPackageJson[prop];
if (typeof field !== 'string') {
// Some crazy packages have things like arrays in these fields!
continue;
}
const relativeTypingsPath = field.replace(/\.js$/, '.d.ts');
const typingsPath = resolve(entryPointPath, relativeTypingsPath);
if (fs.exists(typingsPath)) {
return typingsPath;
}
}
return null;
}
. This seems that this doesn't match the typescript resolution for typings which follows this strategy

/root/src/node_modules/moduleB.ts
/root/src/node_modules/moduleB.tsx
/root/src/node_modules/moduleB.d.ts
/root/src/node_modules/moduleB/package.json (if it specifies a "types" property)
/root/src/node_modules/@types/moduleB.d.ts
/root/src/node_modules/moduleB/index.ts
/root/src/node_modules/moduleB/index.tsx
/root/src/node_modules/moduleB/index.d.ts 
@petebacondarwin

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

@alan-agius4 - we are not 100% inline with TS resolution because these are not TS files. This function is only called if there is no typings or types property in the package.json, which is our first choice and almost always available.

If it is not available then this function is trying to guess the typings file by looking for a typings file that is adjacent to the imported JS file. The lookup therefore looks like:

/root/src/node_modules/moduleB.ts
/root/src/node_modules/moduleB.tsx
/root/src/node_modules/moduleB/package.json (if it specifies a "types" property)
/root/src/node_modules/moduleB.d.ts
/root/src/node_modules/@types/moduleB.d.ts
/root/src/node_modules/moduleB/index.ts
/root/src/node_modules/moduleB/index.tsx
/root/src/node_modules/moduleB/index.d.ts

We do not consider .ts or .tsx files. Also we do not consider @types/moduleB paths. Such paths should only be where a library does not provide its own typings - and this would be very weird (probably broken) for an Angular package.

We could consider supporting the index.d.ts path but so far this has not been a requirement.

ngdevelop-tech added a commit to ngdevelop-tech/angular that referenced this pull request Aug 27, 2019
fix(ngcc): do not analyze dependencies for non Angular entry-points (a…
…ngular#32303)

When ngcc is called for a specific entry-point, it has to determine
which dependencies to transitively process. To accomplish this, ngcc
traverses the full import graph of the entry-points it encounters, for
which it uses a dependency host to find all module imports. Since
imports look different in the various bundle formats ngcc supports, a
specific dependency host is used depending on the information provided
in an entry-points `package.json` file. If there's not enough
information in the `package.json` file for ngcc to be able to determine
which dependency host to use, ngcc would fail with an error.

If, however, the entry-point is not compiled by Angular, it is not
necessary to process any of its dependencies. None of them can have
been compiled by Angular so ngcc does not need to know about them.
Therefore, this commit changes the behavior to avoid recursing into
dependencies of entry-points that are not compiled by Angular.

In particular, this fixes an issue for packages that have dependencies
on the `date-fns` package. This package has various secondary
entry-points that have a `package.json` file only containing a `typings`
field, without providing additional fields for ngcc to know which
dependency host to use. By not needing a dependency host at all, the
error is avoided.

Fixes angular#32302

PR Close angular#32303
@lppedd lppedd referenced this pull request Aug 30, 2019
3 of 3 tasks complete
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
fix(ngcc): do not analyze dependencies for non Angular entry-points (a…
…ngular#32303)

When ngcc is called for a specific entry-point, it has to determine
which dependencies to transitively process. To accomplish this, ngcc
traverses the full import graph of the entry-points it encounters, for
which it uses a dependency host to find all module imports. Since
imports look different in the various bundle formats ngcc supports, a
specific dependency host is used depending on the information provided
in an entry-points `package.json` file. If there's not enough
information in the `package.json` file for ngcc to be able to determine
which dependency host to use, ngcc would fail with an error.

If, however, the entry-point is not compiled by Angular, it is not
necessary to process any of its dependencies. None of them can have
been compiled by Angular so ngcc does not need to know about them.
Therefore, this commit changes the behavior to avoid recursing into
dependencies of entry-points that are not compiled by Angular.

In particular, this fixes an issue for packages that have dependencies
on the `date-fns` package. This package has various secondary
entry-points that have a `package.json` file only containing a `typings`
field, without providing additional fields for ngcc to know which
dependency host to use. By not needing a dependency host at all, the
error is avoided.

Fixes angular#32302

PR Close angular#32303

@JoostK JoostK deleted the JoostK:ngcc-non-angular-entrypoints branch Sep 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.