Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

refactor($parse): do not pass scope,locals to interceptor fns #16091

Merged
merged 1 commit into from
Jul 14, 2017

Conversation

jbedard
Copy link
Collaborator

@jbedard jbedard commented Jul 11, 2017

After @lgalfaso's comment I thought it would be a good idea to remove these params from interceptors. All the internal uses of interceptors are for doing things like data conversion/normalizing, never reading state out of the scope/locals. I think this is the intended use, and makes them like filters (which receive the input value + args).

Note that scope/locals still get passed through to the expression being wrapped/intercepted, it's just the interceptor-fn itself that now only receives the value.

@Narretz
Copy link
Contributor

Narretz commented Jul 11, 2017

@jbedard can you please add the info from the PR to the commit message? This will be helpful for the next person who works on this code. While it's a refactor because it doesn't touch a public API, by including tests we change the expectations for how the internals should work.

@Narretz Narretz added this to the 1.6.6 milestone Jul 11, 2017
All internal use of interceptors are for things such as data
conversion/normalizing, never reading state from the scope/locals.
This is the intended use and makes interceptors more like filters
(which receive only the input value + args, no scope/locals).
@jbedard jbedard force-pushed the parse-interceptor-args branch from 701bb70 to 960ceb1 Compare July 14, 2017 05:50
@jbedard
Copy link
Collaborator Author

jbedard commented Jul 14, 2017

Updated the commit message.

@jbedard
Copy link
Collaborator Author

jbedard commented Jul 14, 2017

I'd also vote to only put this into 1.7 since it does change an internal but publicly accessible API

@Narretz Narretz modified the milestones: 1.7.0, 1.6.6 Jul 14, 2017
@Narretz
Copy link
Contributor

Narretz commented Jul 14, 2017

1.7 is fine with me

@jbedard
Copy link
Collaborator Author

jbedard commented Jul 14, 2017

Commit msg look ok before merging?

@Narretz
Copy link
Contributor

Narretz commented Jul 14, 2017

LGTM

@jbedard jbedard merged commit 529550d into angular:master Jul 14, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants