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(compiler-cli): flat module index metadata should be transformed #23129

Closed
wants to merge 1 commit into from

Conversation

alxhub
Copy link
Member

@alxhub alxhub commented Apr 2, 2018

Currently, the flat module index metadata is produced directly from
the source metadata. The compiler, however, applies transformations
on the Typescript sources during transpilation, and also equivalent
transformations on the metadata itself. This transformed metadata
doesn't end up in the flat module index.

This changes the compiler to generate the flat module index metadata
from its transformed version instead of directly from source.

@@ -1013,10 +1015,26 @@ describe('ngc transformer command-line', () => {
it('should be able to generate a flat module library', () => {
writeFlatModule('index.js');

debugger;
Copy link
Contributor

Choose a reason for hiding this comment

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

debugger here and l1028

Copy link
Contributor

Choose a reason for hiding this comment

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

OMG!!!! 😱

Copy link
Contributor

Choose a reason for hiding this comment

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

just kidding.. I'm sorry, but I couldn't help myself.

Copy link
Contributor

Choose a reason for hiding this comment

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

missing a tslint check for that!

Copy link
Member Author

Choose a reason for hiding this comment

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

Gone!

// contents of the flat module index. The bundle produced during emit does use the metadata cache
// with associated transforms, so the metadata will have lowered expressions, resource inlining,
// etc.
const getMetadataBundle = (cache?: MetadataCache) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think the compiler use explicit null most often. (also in CompilerHostAdapter)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

nits excluding this change looks good to me, but we should wait for @alexeagle to chime in as well.

const normalSyntheticIndexName = path.normalize(syntheticIndex.name);
const indexContent = syntheticIndex.content;
const indexMetadata = syntheticIndex.metadata;
const getIndexMetadata = syntheticIndex.getMetadata;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I don't care much for these extra variables, they introduce an extra indirection just to save some characters?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.


getMetadataFor(fileName: string): ModuleMetadata|undefined {
if (!this.host.fileExists(fileName + '.ts')) return undefined;
const sourceFile = this.host.getSourceFile(fileName + '.ts', ts.ScriptTarget.Latest);
return sourceFile && this.collector.getMetadata(sourceFile);
// If there is a metadata cache, use it to
Copy link
Contributor

Choose a reason for hiding this comment

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

unfinished comment

Copy link
Member Author

Choose a reason for hiding this comment

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

Finished my thought.

@@ -1013,10 +1015,26 @@ describe('ngc transformer command-line', () => {
it('should be able to generate a flat module library', () => {
writeFlatModule('index.js');

debugger;
Copy link
Contributor

Choose a reason for hiding this comment

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

missing a tslint check for that!

Currently, the flat module index metadata is produced directly from
the source metadata. The compiler, however, applies transformations
on the Typescript sources during transpilation, and also equivalent
transformations on the metadata itself. This transformed metadata
doesn't end up in the flat module index.

This changes the compiler to generate the flat module index metadata
from its transformed version instead of directly from source.
@alxhub alxhub added target: major This PR is targeted for the next major release action: merge The PR is ready for merge by the caretaker labels Apr 3, 2018
@alxhub alxhub closed this in f99cb5c Apr 4, 2018
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants