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

Respect flatModuleOutFile option #27497

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@alxhub
Contributor

alxhub commented Dec 6, 2018

No description provided.

@googlebot googlebot added the cla: yes label Dec 6, 2018

@alxhub alxhub force-pushed the alxhub:ngtsc-flat-module-index branch from 12397e6 to 272eef9 Dec 6, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Dec 6, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Dec 6, 2018

@alxhub alxhub force-pushed the alxhub:ngtsc-flat-module-index branch from 272eef9 to cb22a9b Dec 6, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Dec 6, 2018

@alxhub alxhub force-pushed the alxhub:ngtsc-flat-module-index branch from cb22a9b to 31b76f6 Dec 6, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Dec 6, 2018

@IgorMinar

Please rebase and address the comments. otherwise lgtm

env.driveMain();
const dtsContents = env.getContents('flat.d.ts');
expect(dtsContents).toContain('@mymodule');

This comment has been minimized.

@IgorMinar

IgorMinar Dec 6, 2018

Member

what are you actually checking here? can you please use to toBe instead of toContain? The input and output files are so small that checking against the whole file should not be too brittle and could catch serious regressions.

This comment has been minimized.

@alxhub

alxhub Dec 6, 2018

Contributor

I'm trying to assert that the generated .d.ts file for the flat module index contains an appropriate amd-module reference.

I've changed the test to expect .toContain('/// <amd-module name="@mymodule" />'), which is the full text that we expect to appear. I don't want to check the entire file as I really try to avoid tests which assert things other than their main purpose (for example, this test should not be sensitive to the text of the marker comment we add to the file).

rootFiles.push(flatIndexGenerator.flatIndexPath);
} else {
throw new Error(
'Angular compiler option "flatModuleIndex" requires one and only one .ts file in the "files" field.');

This comment has been minimized.

@IgorMinar

IgorMinar Dec 6, 2018

Member

this error message is now consistent with the check in the forRoot where we are looking for index.ts with the shortest path. can you update this?

This comment has been minimized.

@alxhub

alxhub Dec 6, 2018

Contributor

It's intended to be consistent with the same check in ngc (https://github.com/angular/angular/blob/master/packages/compiler-cli/src/metadata/bundle_index_host.ts#L91-L92). The shortest-path behavior is deprecated and we shouldn't encourage users to use it (not that this stuff was documented at all anyway)

This comment has been minimized.

@IgorMinar

IgorMinar Dec 6, 2018

Member

This is weird. We should talk about it. At minimum we need to document this oddity in the source code.

This comment has been minimized.

@alxhub

alxhub Dec 6, 2018

Contributor

I added some documentation in the comments.

@IgorMinar

This comment has been minimized.

Member

IgorMinar commented Dec 6, 2018

hmm... also CI is unhappy

@IgorMinar

This comment has been minimized.

Member

IgorMinar commented Dec 6, 2018

and lastly - if this was implemented correctly, you should be able to remove this fixme:

"fixme-ivy-aot", # ivy no longer emits generated index file

refactor(ivy): allow for shim generators not based on existing files
Previously the ngtsc ShimGenerator interface expected that all shims would
be generated using the contents of existing ts.SourceFiles. This assumption
was true for ngfactory and ngsummary files, but breaks down for flat module
index files, which are standalone.

This commit prepares for flat module index generation by enabling shim
generators which don't require an existing file.

@alxhub alxhub force-pushed the alxhub:ngtsc-flat-module-index branch from 31b76f6 to 542b008 Dec 6, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Dec 6, 2018

@alxhub alxhub force-pushed the alxhub:ngtsc-flat-module-index branch from 542b008 to 8141156 Dec 6, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Dec 6, 2018

@alxhub alxhub force-pushed the alxhub:ngtsc-flat-module-index branch 2 times, most recently from 310faf5 to 754230a Dec 6, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Dec 6, 2018

feat(ivy): generate flat module index files
Previously, ngtsc did not respect the angularCompilerOptions settings
for generating flat module indices. This commit adds a
FlatIndexGenerator which is used to implement those options.

FW-738 #resolve

@alxhub alxhub force-pushed the alxhub:ngtsc-flat-module-index branch from 754230a to 368dbfc Dec 6, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Dec 6, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Dec 6, 2018

@IgorMinar

one nit. otherwise lgtm

static forRootFiles(flatIndexPath: string, files: ReadonlyArray<string>, moduleName: string|null):
FlatIndexGenerator|null {
// If there's only one .ts file in the program, it's the entry. Otherwise, look for the shortest

This comment has been minimized.

@IgorMinar

IgorMinar Dec 6, 2018

Member

move the "Otherwise..." part to the else branch for better discoverability.

@alxhub alxhub closed this in 352c582 Dec 7, 2018

alxhub added a commit that referenced this pull request Dec 7, 2018

feat(ivy): generate flat module index files (#27497)
Previously, ngtsc did not respect the angularCompilerOptions settings
for generating flat module indices. This commit adds a
FlatIndexGenerator which is used to implement those options.

FW-738 #resolve

PR Close #27497
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment