Skip to content

feat(core): Add a public API to establish events to be replayed and a…#55356

Closed
iteriani wants to merge 1 commit intoangular:mainfrom
iteriani:publicapi3
Closed

feat(core): Add a public API to establish events to be replayed and a…#55356
iteriani wants to merge 1 commit intoangular:mainfrom
iteriani:publicapi3

Conversation

@iteriani
Copy link
Contributor

…n attribute to mark an element with an event handler.

These will be consumed by the event-dispatch contract to replay events. The contract and the dispatcher inclusion will be in followups.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@pullapprove pullapprove bot requested review from atscott and crisbeto April 16, 2024 01:40
@iteriani iteriani requested review from tbondwilkinson and removed request for atscott and crisbeto April 16, 2024 01:40
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Apr 16, 2024
@pullapprove pullapprove bot requested review from atscott and crisbeto April 16, 2024 01:40
@iteriani iteriani requested review from rahatarmanahmed and removed request for atscott and crisbeto April 16, 2024 01:41
@pullapprove pullapprove bot requested review from atscott and crisbeto April 16, 2024 01:41
@iteriani iteriani requested review from AndrewKushnir and alan-agius4 and removed request for atscott and crisbeto April 16, 2024 01:41
@pkozlowski-opensource pkozlowski-opensource added the area: core Issues related to the framework runtime label Apr 16, 2024
@ngbot ngbot bot added this to the Backlog milestone Apr 16, 2024
Comment on lines 64 to 66
Copy link
Contributor

Choose a reason for hiding this comment

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

We can refactor (simplify) this code a bit to change the type from a string to a boolean:

Suggested change
const type =
(typeof useCaptureOrIndx === 'boolean' || useCaptureOrIndx >= 0) ? 'dom' : 'output';
if (type === 'dom') {
const isDomEvent = typeof useCaptureOrIndx === 'boolean' || useCaptureOrIndx >= 0;
if (isDomEvent) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to make JSON data structures not have XSS vulnerabilities?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is pretty confusing. If I'm reading it correctly, it sounds like you're doing something like this:

const isCaptureArg = typeof useCaptureOrIndx === 'boolean'
const isUnsubscribeMethod = usecaptureOrIndx >= 0;
if (isCaptureArg || isUnsubscribeMethod) {
  ...
}

But also I don't really get why we only add the event to the contract in those two conditions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is copy paste from the prototype. maybe @AndrewKushnir can help out here.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK actually I think I get it now. I'm guessing the unsubscribe method is like an observable for the event or something?

@AndrewKushnir AndrewKushnir removed the action: merge The PR is ready for merge by the caretaker label Apr 18, 2024
@iteriani
Copy link
Contributor Author

Caretaker note: this PR requires cl/625907131 to be submitted first.

@iteriani iteriani added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Apr 18, 2024
@pullapprove pullapprove bot requested review from alxhub and devversion April 18, 2024 19:37
@pullapprove pullapprove bot added the requires: TGP This PR requires a passing TGP before merging is allowed label Apr 18, 2024
@angular-robot angular-robot bot added detected: breaking change PR contains a commit with a breaking change area: docs Related to the documentation area: build & ci Related the build and CI infrastructure of the project labels Apr 18, 2024
@pullapprove pullapprove bot removed the requires: TGP This PR requires a passing TGP before merging is allowed label Apr 18, 2024
@angular-robot angular-robot bot removed detected: breaking change PR contains a commit with a breaking change area: docs Related to the documentation area: build & ci Related the build and CI infrastructure of the project labels Apr 18, 2024
@iteriani iteriani force-pushed the publicapi3 branch 2 times, most recently from 0510faa to 85dbc13 Compare April 19, 2024 14:05
…n attribute to mark an element with an event handler.

These will be consumed by the event-dispatch contract to replay events. The contract and the dispatcher inclusion will be in followups.
@AndrewKushnir
Copy link
Contributor

Caretaker note: presubmit is "green", this PR is ready for merge.

@alxhub
Copy link
Member

alxhub commented Apr 19, 2024

This PR was merged into the repository by commit a730f09.

@alxhub alxhub closed this in a730f09 Apr 19, 2024
@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.

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 detected: feature PR contains a feature commit merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants