Skip to content

fix(compiler-cli): produce metadata for .d.ts files without metadata#13526

Merged
chuckjaz merged 1 commit intoangular:masterfrom
chuckjaz:issue13307
Dec 16, 2016
Merged

fix(compiler-cli): produce metadata for .d.ts files without metadata#13526
chuckjaz merged 1 commit intoangular:masterfrom
chuckjaz:issue13307

Conversation

@chuckjaz
Copy link
Copy Markdown
Contributor

@chuckjaz chuckjaz commented Dec 16, 2016

What kind of change does this PR introduce? (check one with "x")

[x] Bugfix

What is the current behavior? (You can also link to an open issue here)

All .d.ts files without metadata in v1 were considered to not have metadata for v3 so re-export information was lost and symbols reexported by these modules could not be resolved.

What is the new behavior?

A .d.ts file with no metadata will get metadata from the .d.ts file which will include the re-exports in that file.

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

Other information:

Fixes #13307

@chuckjaz chuckjaz requested a review from tbosch December 16, 2016 19:13
@chuckjaz chuckjaz mentioned this pull request Dec 16, 2016
Closed
@chuckjaz chuckjaz force-pushed the issue13307 branch 2 times, most recently from 2fec6d8 to 53372e0 Compare December 16, 2016 22:34
vicb
vicb reviewed Dec 16, 2016
View reviewed changes
modules/@angular/compiler-cli/src/compiler_host.ts Outdated
Copy link
Copy Markdown
Contributor

@vicb vicb Dec 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

+ add brief comment why no dts is equivalent to module v1

vicb
vicb reviewed Dec 16, 2016
View reviewed changes
modules/@angular/compiler-cli/src/compiler_host.ts Outdated
Copy link
Copy Markdown
Contributor

@vicb vicb Dec 16, 2016

Choose a reason for hiding this comment

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

Is this considering l. 186 ? This should never be minified

Copy link
Copy Markdown
Contributor Author

@chuckjaz chuckjaz Dec 16, 2016

Choose a reason for hiding this comment

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

This code should never be minified through an aggressive minifier like closure so these are equivalent.

vicb
vicb reviewed Dec 16, 2016
View reviewed changes
tools/@angular/tsc-wrapped/src/collector.ts Outdated
Copy link
Copy Markdown
Contributor

@vicb vicb Dec 16, 2016

Choose a reason for hiding this comment

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

|| identifier.text is this required ?

Copy link
Copy Markdown
Contributor

@vicb vicb Dec 16, 2016

Choose a reason for hiding this comment

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

ok got it, nevermind

Copy link
Copy Markdown
Contributor Author

@chuckjaz chuckjaz Dec 16, 2016

Choose a reason for hiding this comment

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

Yes. If the identifier is not renamed on export then it has its natural name.

vicb
vicb reviewed Dec 16, 2016
View reviewed changes
tools/@angular/tsc-wrapped/src/collector.ts Outdated
Copy link
Copy Markdown
Contributor

@vicb vicb Dec 16, 2016

Choose a reason for hiding this comment

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

use isExported

Copy link
Copy Markdown
Contributor Author

@chuckjaz chuckjaz Dec 16, 2016

Choose a reason for hiding this comment

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

We cannot here because a variable declaration doesn't have a name but might have a name list. So instead of making isExported complicated I left the special case here.

vicb
vicb approved these changes Dec 16, 2016
View reviewed changes
Copy link
Copy Markdown
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

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

LGTM with minor comments

@chuckjaz chuckjaz added action: review The PR is still awaiting reviews from at least one requested reviewer pr_state: LGTM action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 16, 2016
@chuckjaz chuckjaz merged commit 0e3981a into angular:master Dec 16, 2016
chuckjaz added a commit that referenced this pull request Dec 20, 2016
@chuckjaz
fix(compiler-cli): produce metadata for .d.ts files without metadata (#…
debb0c9 
…13526)

Fixes #13307
Fixes #13473
Fixes #13521
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented Sep 10, 2019

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 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@tbosch tbosch Awaiting requested review from tbosch

1 more reviewer

@vicb vicb vicb approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Regression when using static method & ANALYZE_FOR_ENTRY_COMPONENTS

3 participants

@chuckjaz @vicb @googlebot