Skip to content

build: TypeScript 3.5 upgrade#31615

Closed
IgorMinar wants to merge 1 commit intoangular:masterfrom
IgorMinar:build/ts-3.5-upgrade
Closed

build: TypeScript 3.5 upgrade#31615
IgorMinar wants to merge 1 commit intoangular:masterfrom
IgorMinar:build/ts-3.5-upgrade

Conversation

@IgorMinar
Copy link
Copy Markdown
Contributor

@IgorMinar IgorMinar requested review from a team July 18, 2019 00:50
@mary-poppins
Copy link
Copy Markdown

@IgorMinar
Copy link
Copy Markdown
Contributor Author

//packages/compiler-cli/test/ngtsc:ngtsc is failing due to compiler host not being passed properly somewhere in between our test setup, tsickle, and typescript. My guess is that the test doesn't properly initialize the environment. @alxhub is this something you could look into please? Feel free to push to this branch.

@EatonZ
Copy link
Copy Markdown

EatonZ commented Jul 18, 2019

TypesScript should be TypeScript (no extra s) in the title.😉

@googlebot
Copy link
Copy Markdown

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only "I consent." in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@mary-poppins
Copy link
Copy Markdown

@IgorMinar IgorMinar requested a review from a team July 18, 2019 23:36
@IgorMinar IgorMinar force-pushed the build/ts-3.5-upgrade branch from bd8d99f to 86a3fd6 Compare July 18, 2019 23:38
@mary-poppins
Copy link
Copy Markdown

@mary-poppins
Copy link
Copy Markdown

@mary-poppins
Copy link
Copy Markdown

@mary-poppins
Copy link
Copy Markdown

@IgorMinar IgorMinar changed the title build: TypesScript 3.5 upgrade build: TypeScript 3.5 upgrade Jul 25, 2019
@IgorMinar
Copy link
Copy Markdown
Contributor Author

@EatonZ :-D thanks for pointing that out

@mary-poppins
Copy link
Copy Markdown

@mary-poppins
Copy link
Copy Markdown

@IgorMinar IgorMinar force-pushed the build/ts-3.5-upgrade branch from d0ceee3 to 2a07df6 Compare July 25, 2019 22:52
@googlebot
Copy link
Copy Markdown

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@IgorMinar IgorMinar force-pushed the build/ts-3.5-upgrade branch from 2a07df6 to e390cda Compare July 25, 2019 22:52
@mary-poppins
Copy link
Copy Markdown

@mary-poppins
Copy link
Copy Markdown

@IgorMinar IgorMinar force-pushed the build/ts-3.5-upgrade branch 2 times, most recently from 82b374b to e538486 Compare July 25, 2019 23:07
@IgorMinar IgorMinar requested a review from a team July 25, 2019 23:07
@IgorMinar IgorMinar force-pushed the build/ts-3.5-upgrade branch from e538486 to 6fff209 Compare July 25, 2019 23:08
@mary-poppins
Copy link
Copy Markdown

@mary-poppins
Copy link
Copy Markdown

@mary-poppins
Copy link
Copy Markdown

@mhevery mhevery added the area: core Issues related to the framework runtime label Jul 25, 2019
@ngbot ngbot Bot added this to the needsTriage milestone Jul 25, 2019
Copy link
Copy Markdown
Contributor Author

@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.

notes on the changes I make to make the review easier.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is just a spec and the legacy ngc internal types are off, so any seems like the best way to go

Comment thread packages/core/src/event_emitter.ts Outdated
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

T otherwise end up defaults to unknown

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is not a public api so it's ok to change and correct

*/
export const MOCK_PLATFORM_LOCATION_CONFIG = new InjectionToken('MOCK_PLATFORM_LOCATION_CONFIG');
export const MOCK_PLATFORM_LOCATION_CONFIG =
new InjectionToken<MockPlatformLocationConfig>('MOCK_PLATFORM_LOCATION_CONFIG');
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this makes the type more precise but I don't expect this to break anyone, the alternative is unknown which is likely more breaky.

'<div>{{"hello" | aPipe}}</div>',
`Argument of type '"hello"' is not assignable to parameter of type 'number'.`, '0:5');
});
it('should report an index into a map expression', () => {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this error is now being caught by typescript, so the test fails because the diagnostic comes from tsc rather than ngc. that's why I removed the test.

// i.e. users have to ask for what they need. With that, we can build better analysis tools
// and could do better codegen in the future.
export class ElementRef<T = any> {
export class ElementRef<T extends any = any> {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

not ideal, but the goal was to make this backwards compatible now that unspecified generics result in unknown and the default value is not being honored.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would this not extend Node???

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

because that would make it much more strict than what we have today and would break even those people that are not upgrading typescript.

we can update this stuff in v9 to a more precise type, but we can't do it in 8.2

@IgorMinar IgorMinar added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release and removed state: WIP labels Jul 25, 2019
// i.e. users have to ask for what they need. With that, we can build better analysis tools
// and could do better codegen in the future.
export class ElementRef<T = any> {
export class ElementRef<T extends any = any> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would this not extend Node???

@IgorMinar IgorMinar added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Jul 25, 2019
@IgorMinar
Copy link
Copy Markdown
Contributor Author

merge-assistance: global approval

@angular-automatic-lock-bot
Copy link
Copy Markdown

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 15, 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 area: core Issues related to the framework runtime cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TypeScript 3.5 Support

5 participants