Skip to content

Conversation

tbosch
Copy link
Contributor

@tbosch tbosch commented Aug 31, 2017

No description provided.

@mary-poppins
Copy link

You can preview 18416e2 at https://pr18986-18416e2.ngbuilds.io/.

@tbosch tbosch force-pushed the fix_g3_3 branch 2 times, most recently from f82e083 to a3d19d7 Compare August 31, 2017 21:32
@mary-poppins
Copy link

You can preview a3d19d7 at https://pr18986-a3d19d7.ngbuilds.io/.

@mary-poppins
Copy link

You can preview d728a0b at https://pr18986-d728a0b.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 496644a at https://pr18986-496644a.ngbuilds.io/.

@@ -106,6 +106,7 @@ def _compile_action(ctx, inputs, outputs, config_file_path):
if hasattr(ctx.attr, "tsconfig") and ctx.file.tsconfig:
action_inputs += [ctx.file.tsconfig]

arguments = ["--node_options=--expose-gc"]
# One at-sign makes this a params-file, enabling the worker strategy.
# Two at-signs escapes the argument so it's passed through to ngc
# rather than the contents getting expanded.
Copy link
Contributor

Choose a reason for hiding this comment

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

missing the += below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

export function compile(
{fileLoader, compilerOpts, bazelOpts, files, expectedOuts, gatherDiagnostics}: {
Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure about taking named parameters here? so clunky

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This is way easier to read on the call site than to take all of these as positional arguments. We have it in the ngc api as well. And chuck also complained about it :-)

@@ -28,6 +28,11 @@ describe('Expression lowering', () => {
class MyClass {}
`)).toContain('const l = () => null; exports.l = l;');
});

it('should be able to export a variable if the whole value is lowered', () => {
expect(convert('/*a*/ const a =◊b: () => null◊;'))
Copy link
Contributor

Choose a reason for hiding this comment

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

weird diamond characters appear on github web ui :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is on purpose. See the other tests.

@mary-poppins
Copy link

You can preview a8d9597 at https://pr18986-a8d9597.ngbuilds.io/.

@mary-poppins
Copy link

You can preview c1ca788 at https://pr18986-c1ca788.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 1dda6ce at https://pr18986-1dda6ce.ngbuilds.io/.

…flexible

Needed to allow custom checking for diagnostics.
This e.g. leaves comments at the right place, which is important for closure.
@IgorMinar IgorMinar self-requested a review September 7, 2017 04:28
@mary-poppins
Copy link

You can preview 4b3cbb3 at https://pr18986-4b3cbb3.ngbuilds.io/.

@tbosch tbosch added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release area: core Issues related to the framework runtime labels Sep 7, 2017
@matsko
Copy link
Contributor

matsko commented Sep 8, 2017

Landed as a69172f

@matsko matsko closed this Sep 8, 2017
@tbosch tbosch deleted the fix_g3_3 branch September 19, 2017 23:20
@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 12, 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 target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants