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

refactor(compiler): create a new root BindingScope for each template #36362

Conversation

petebacondarwin
Copy link
Member

Previously we had a singleton ROOT_SCOPE object, from
which all BindingScopes derived. But this caused ngcc to
produce non-deterministic output when running multiple workers
in parallel, since each process had its own ROOT_SCOPE.

In reality there is no need for BindingScope reference names
to be unique across an entire application (or in the case of ngcc
across all the libraries). Instead we just need uniqueness within
a template.

This commit changes the compiler to create a new root BindingScope
each time it compiles a component's template.

Resolves #35180

Previously we had a singleton `ROOT_SCOPE` object, from
which all `BindingScope`s derived. But this caused ngcc to
produce non-deterministic output when running multiple workers
in parallel, since each process had its own `ROOT_SCOPE`.

In reality there is no need for `BindingScope` reference names
to be unique across an entire application (or in the case of ngcc
across all the libraries). Instead we just need uniqueness within
a template.

This commit changes the compiler to create a new root `BindingScope`
each time it compiles a component's template.

Resolves angular#35180
@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release comp: ngcc area: compiler Issues related to `ngc`, Angular's template compiler labels Apr 1, 2020
@ngbot ngbot bot added this to the needsTriage milestone Apr 1, 2020
@petebacondarwin
Copy link
Member Author

I couldn't find any suitable place to add additional tests of this change; and since it has no real change to the behaviour of the generated code I decided it was really just a refactor (or perhaps a build related change).

@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 8, 2020
@alxhub
Copy link
Member

alxhub commented Apr 8, 2020

Presubmit

@petebacondarwin petebacondarwin removed the action: presubmit The PR is in need of a google3 presubmit label Apr 9, 2020
@atscott atscott closed this in e526f74 Apr 9, 2020
atscott pushed a commit that referenced this pull request Apr 9, 2020
#36362)

Previously we had a singleton `ROOT_SCOPE` object, from
which all `BindingScope`s derived. But this caused ngcc to
produce non-deterministic output when running multiple workers
in parallel, since each process had its own `ROOT_SCOPE`.

In reality there is no need for `BindingScope` reference names
to be unique across an entire application (or in the case of ngcc
across all the libraries). Instead we just need uniqueness within
a template.

This commit changes the compiler to create a new root `BindingScope`
each time it compiles a component's template.

Resolves #35180

PR Close #36362
@petebacondarwin petebacondarwin deleted the ngcc-deterministic-builds branch April 9, 2020 16:57
@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 May 10, 2020
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: compiler Issues related to `ngc`, Angular's template compiler cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ngcc is non-deterministic
3 participants