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

Fixes needed for NativeScript support in ngcc #33192

Closed
wants to merge 5 commits into from

Conversation

@alxhub
Copy link
Contributor

alxhub commented Oct 16, 2019

This PR groups together a number of fixes required to support nativescript-angular and other such packages in ngcc.

@googlebot googlebot added the cla: yes label Oct 16, 2019
@alxhub alxhub requested review from gkalpak and petebacondarwin Oct 16, 2019
@alxhub alxhub marked this pull request as ready for review Oct 16, 2019
@alxhub alxhub requested review from angular/fw-compiler as code owners Oct 16, 2019
Copy link
Member

gkalpak left a comment

Some minor comments/questions. Otherwise 🚀

type: 'boolean',
})
.option('async', {
describe: 'Whether to compile asynchronously',

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 17, 2019

Member

Missing trailing . (for consistency 😁).

I would consider adding some more information on what this option is good for (e.g. mentioning that async mode enables some optimization (such as parallelization) that can improve performance).

This comment has been minimized.

Copy link
@alxhub

alxhub Oct 17, 2019

Author Contributor

👍

@@ -174,7 +174,8 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost {
return null;
}

return {node: importedFile, viaModule: importInfo.from};
const viaModule = !importInfo.from.startsWith('./') ? importInfo.from : null;

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 17, 2019

Member

What about just '.'? (Wouldn't that be equivalent to './index.js' or is this only a thing in Node.js?)

This comment has been minimized.

Copy link
@alxhub

alxhub Oct 17, 2019

Author Contributor

Fixed.

@@ -43,6 +43,8 @@ export interface NgccEntryPointConfig {
* entry-point's package.json file.
*/
override?: PackageJsonFormatPropertiesMap;

ignoreMissingDependencies?: boolean;

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 17, 2019

Member

Docs please 🙏 😃

This comment has been minimized.

Copy link
@alxhub

alxhub Oct 17, 2019

Author Contributor

Done.

expect(result.entryPoints).toEqual([sixthIgnoreMissing, first]);
expect(result.invalidEntryPoints).toEqual([]);
});

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 17, 2019

Member

Could you add another test to verify what happens in the folloing situation:

spyOn(host, 'findDependencies').and.callFake(createFakeComputeDependencies({
  [_('/first/index.js')]: {resolved: [], missing: ['/missing']},
  [_('/second/sub/index.js')]: {resolved: [first.path], missing: []},
  [_('/sixth/index.js')]: {resolved: [second.path], missing: []},
}));
// Note that we will process `first` after `second`, which has the missing dependency.
const result = resolver.sortEntryPointsByDependency([first, second, sixthIgnoreMissing]);
expect(result.entryPoints).toEqual([sixthIgnoreMissing]);  // <-- Is this what actually happens?
expect(result.invalidEntryPoints).toEqual([
  ...
]);

This comment has been minimized.

Copy link
@alxhub

alxhub Oct 17, 2019

Author Contributor

I don't believe this is what happens, because sixth does have its (shallow) dependencies. But, one of them is invalid (because it itself has missing dependencies) and so sixth is skipped too - which is in line with the meaning of the flag.

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 17, 2019

Member

Yeah, I wasn't sure what should happen. But the point was that whatever happens should be verified by a test 😃

This comment has been minimized.

Copy link
@alxhub

alxhub Oct 17, 2019

Author Contributor

I agree :D and now it is!

@@ -214,3 +222,12 @@ function generateImportString(
const importAs = importPath ? importManager.generateNamedImport(importPath, importName) : null;
return importAs ? `${importAs.moduleImport}.${importAs.symbol}` : `${importName}`;
}

function getNextSiblingInArray<T extends ts.Node>(node: T, array: ts.NodeArray<T>): T|null {
for (let i = 0; i < array.length; i++) {

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 17, 2019

Member

OOC, why not use array.indexOf(node)?

This comment has been minimized.

Copy link
@alxhub

alxhub Oct 17, 2019

Author Contributor

Done.

decoratorsToRemove.set(decorator.node.parent !, [decorator.node]);
renderer.removeDecorators(output, decoratorsToRemove);
// The decorator should have been removed correctly.
expect(output.toString()).toContain('A.decorators = [ { type: OtherA }');

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 17, 2019

Member

I am a little confused. What would happen before this commit?

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 17, 2019

Member

I think that actually instead of this test, we should fix the earlier test ("should delete the decorator (and following comma) that was matched in the analysis"), since that is not correctly testing that the trailing comma was removed.

I believe that previous to this commit the generated code looks like:

A.decorators = [ , { type: OtherA } 

Notice that the trailing comma from the removed decorator was not removed.

This comment has been minimized.

Copy link
@alxhub

alxhub Oct 17, 2019

Author Contributor

The previous test has value, because it's testing that the comma is removed in the general case.

This test verifies that the comma is removed in the special case where the decorator has a trailing comment (which messed up the previous logic).

import {LogicalFileSystem, absoluteFrom} from '../../file_system';
import {runInEachFileSystem} from '../../file_system/testing';
import {getDeclaration, makeProgram} from '../../testing';
import {Declaration, TypeScriptReflectionHost} from '.././../reflection';

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 17, 2019

Member

Funky ./ path segment 🤔

This comment has been minimized.

Copy link
@alxhub

alxhub Oct 17, 2019

Author Contributor

Gone :)

// First, get the exports of the file.
const symbol = checker.getSymbolAtLocation(file);
if (symbol === undefined) {
target: ts.Node, file: ts.SourceFile, checker: ts.TypeChecker,

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 17, 2019

Member

Is checker needed now?

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 17, 2019

Member

Good call @gkalpak - in fact it would be a good idea to try to remove all use of checker from any code outside a ReflectionHost.

This comment has been minimized.

Copy link
@alxhub

alxhub Oct 17, 2019

Author Contributor

Done.

Copy link
Member

petebacondarwin left a comment

I agree with @gkalpak's comments.
I would like to see the missing dependency config more fine grained.
Otherwise LGTM

@@ -43,6 +43,8 @@ export interface NgccEntryPointConfig {
* entry-point's package.json file.
*/
override?: PackageJsonFormatPropertiesMap;

ignoreMissingDependencies?: boolean;

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 17, 2019

Member

I feel it would be better to have a list of dependencies to ignore.
Even if a package has some dependencies that we don't care about we should still fail if the dependencies we do care about are missing.

decoratorsToRemove.set(decorator.node.parent !, [decorator.node]);
renderer.removeDecorators(output, decoratorsToRemove);
// The decorator should have been removed correctly.
expect(output.toString()).toContain('A.decorators = [ { type: OtherA }');

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 17, 2019

Member

I think that actually instead of this test, we should fix the earlier test ("should delete the decorator (and following comma) that was matched in the analysis"), since that is not correctly testing that the trailing comma was removed.

I believe that previous to this commit the generated code looks like:

A.decorators = [ , { type: OtherA } 

Notice that the trailing comma from the removed decorator was not removed.

// Look for the export which declares the node.
const found = exports.find(sym => symbolDeclaresNode(sym, target, checker));
if (found === undefined) {
const keys = Array.from(exports.keys());

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 17, 2019

Member

Why not iterate over the .values() here instead of .keys() then you would not need to do the decl = exports.get(key)` statement.

This comment has been minimized.

Copy link
@alxhub

alxhub Oct 17, 2019

Author Contributor

Because we're calling find on the array of keys to find the name.

This comment has been minimized.

Copy link
@petebacondarwin
// First, get the exports of the file.
const symbol = checker.getSymbolAtLocation(file);
if (symbol === undefined) {
target: ts.Node, file: ts.SourceFile, checker: ts.TypeChecker,

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 17, 2019

Member

Good call @gkalpak - in fact it would be a good idea to try to remove all use of checker from any code outside a ReflectionHost.

@alxhub alxhub force-pushed the alxhub:ngcc/nativescript-fixes branch from 9668994 to 31a1f23 Oct 17, 2019
alxhub added 5 commits Oct 1, 2019
This allows disabling parallelism in ngcc if desired, which is mainly useful
for debugging. The implementation creates the flag and passes its value to
mainNgcc.

No tests are added since the feature mainly exists already - ngcc supports
both parallel and serial execution. This commit only allows switching the
flag via the commandline.
In the ReflectionHost API, a 'viaModule' indicates that a particular value
originated in another absolute module. It should always be 'null' for values
originating in relatively-imported modules.

This commit fixes a bug in the CommonJsReflectionHost where viaModule would
be reported even for relatively-imported values, which causes invalid import
statements to be generated during compilation.

A test is added to verify the correct behavior.

FW-1628 #resolve
Normally, when ngcc encounters a package with missing dependencies while
attempting to determine a compilation ordering, it will ignore that package.
This commit adds a configuration for a flag to tell ngcc to compile the
package anyway, regardless of any missing dependencies.

FW-1931 #resolve
for removal of decorator from __decorate calls.

FW-1629 #resolve
This commit fixes ngtsc's import generator to use the ReflectionHost when
looking through the exports of an ES module to find the export of a
particular declaration that's being imported. This is necessary because
some module formats like CommonJS have unusual export mechanics, and the
normal TypeScript ts.TypeChecker does not understand them.

This fixes an issue with ngcc + CommonJS where exports were not being
enumerated correctly.

FW-1630 #resolve
@alxhub alxhub force-pushed the alxhub:ngcc/nativescript-fixes branch from 31a1f23 to 62968b0 Oct 17, 2019
@alxhub

This comment has been minimized.

Copy link
Contributor Author

alxhub commented Oct 17, 2019

@alxhub alxhub dismissed gkalpak’s stale review Oct 17, 2019

Comments addressed!

@matsko matsko added the comp: ivy label Oct 17, 2019
@ngbot ngbot bot added this to the needsTriage milestone Oct 17, 2019
@matsko matsko closed this in 2196114 Oct 17, 2019
matsko added a commit that referenced this pull request Oct 17, 2019
…#33192)

In the ReflectionHost API, a 'viaModule' indicates that a particular value
originated in another absolute module. It should always be 'null' for values
originating in relatively-imported modules.

This commit fixes a bug in the CommonJsReflectionHost where viaModule would
be reported even for relatively-imported values, which causes invalid import
statements to be generated during compilation.

A test is added to verify the correct behavior.

FW-1628 #resolve

PR Close #33192
matsko added a commit that referenced this pull request Oct 17, 2019
Normally, when ngcc encounters a package with missing dependencies while
attempting to determine a compilation ordering, it will ignore that package.
This commit adds a configuration for a flag to tell ngcc to compile the
package anyway, regardless of any missing dependencies.

FW-1931 #resolve

PR Close #33192
matsko added a commit that referenced this pull request Oct 17, 2019
for removal of decorator from __decorate calls.

FW-1629 #resolve

PR Close #33192
matsko added a commit that referenced this pull request Oct 17, 2019
…33192)

This commit fixes ngtsc's import generator to use the ReflectionHost when
looking through the exports of an ES module to find the export of a
particular declaration that's being imported. This is necessary because
some module formats like CommonJS have unusual export mechanics, and the
normal TypeScript ts.TypeChecker does not understand them.

This fixes an issue with ngcc + CommonJS where exports were not being
enumerated correctly.

FW-1630 #resolve

PR Close #33192
@mgechev mgechev referenced this pull request Oct 18, 2019
22 of 45 tasks complete
ODAVING added a commit to ODAVING/angular that referenced this pull request Oct 18, 2019
This allows disabling parallelism in ngcc if desired, which is mainly useful
for debugging. The implementation creates the flag and passes its value to
mainNgcc.

No tests are added since the feature mainly exists already - ngcc supports
both parallel and serial execution. This commit only allows switching the
flag via the commandline.

PR Close angular#33192
ODAVING added a commit to ODAVING/angular that referenced this pull request Oct 18, 2019
…angular#33192)

In the ReflectionHost API, a 'viaModule' indicates that a particular value
originated in another absolute module. It should always be 'null' for values
originating in relatively-imported modules.

This commit fixes a bug in the CommonJsReflectionHost where viaModule would
be reported even for relatively-imported values, which causes invalid import
statements to be generated during compilation.

A test is added to verify the correct behavior.

FW-1628 #resolve

PR Close angular#33192
ODAVING added a commit to ODAVING/angular that referenced this pull request Oct 18, 2019
…#33192)

Normally, when ngcc encounters a package with missing dependencies while
attempting to determine a compilation ordering, it will ignore that package.
This commit adds a configuration for a flag to tell ngcc to compile the
package anyway, regardless of any missing dependencies.

FW-1931 #resolve

PR Close angular#33192
ODAVING added a commit to ODAVING/angular that referenced this pull request Oct 18, 2019
…3192)

for removal of decorator from __decorate calls.

FW-1629 #resolve

PR Close angular#33192
ODAVING added a commit to ODAVING/angular that referenced this pull request Oct 18, 2019
…ngular#33192)

This commit fixes ngtsc's import generator to use the ReflectionHost when
looking through the exports of an ES module to find the export of a
particular declaration that's being imported. This is necessary because
some module formats like CommonJS have unusual export mechanics, and the
normal TypeScript ts.TypeChecker does not understand them.

This fixes an issue with ngcc + CommonJS where exports were not being
enumerated correctly.

FW-1630 #resolve

PR Close angular#33192
AndrusGerman added a commit to AndrusGerman/angular that referenced this pull request Oct 22, 2019
This allows disabling parallelism in ngcc if desired, which is mainly useful
for debugging. The implementation creates the flag and passes its value to
mainNgcc.

No tests are added since the feature mainly exists already - ngcc supports
both parallel and serial execution. This commit only allows switching the
flag via the commandline.

PR Close angular#33192
AndrusGerman added a commit to AndrusGerman/angular that referenced this pull request Oct 22, 2019
…angular#33192)

In the ReflectionHost API, a 'viaModule' indicates that a particular value
originated in another absolute module. It should always be 'null' for values
originating in relatively-imported modules.

This commit fixes a bug in the CommonJsReflectionHost where viaModule would
be reported even for relatively-imported values, which causes invalid import
statements to be generated during compilation.

A test is added to verify the correct behavior.

FW-1628 #resolve

PR Close angular#33192
AndrusGerman added a commit to AndrusGerman/angular that referenced this pull request Oct 22, 2019
…#33192)

Normally, when ngcc encounters a package with missing dependencies while
attempting to determine a compilation ordering, it will ignore that package.
This commit adds a configuration for a flag to tell ngcc to compile the
package anyway, regardless of any missing dependencies.

FW-1931 #resolve

PR Close angular#33192
AndrusGerman added a commit to AndrusGerman/angular that referenced this pull request Oct 22, 2019
…3192)

for removal of decorator from __decorate calls.

FW-1629 #resolve

PR Close angular#33192
AndrusGerman added a commit to AndrusGerman/angular that referenced this pull request Oct 22, 2019
…ngular#33192)

This commit fixes ngtsc's import generator to use the ReflectionHost when
looking through the exports of an ES module to find the export of a
particular declaration that's being imported. This is necessary because
some module formats like CommonJS have unusual export mechanics, and the
normal TypeScript ts.TypeChecker does not understand them.

This fixes an issue with ngcc + CommonJS where exports were not being
enumerated correctly.

FW-1630 #resolve

PR Close angular#33192
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.