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(core): usage strategy should detect ambiguous query usage #30215

Closed

Conversation

@devversion
Copy link
Member

commented Apr 30, 2019

Currently we always just set the timing to false if we aren't
able to analyze a given call expression or new expression. e.g.

ngOnInit() {
  thirdPartyCallSync(() => this.query.doSomething())
}

In that case the thirdPartyCallSync function comes from the node_modules
and is only defined through types while there is no code for the
actual function logic that can be analyzed. This makes it impossible
to tell whether the given call expression actually causes the specified
arrow function to be executed synchronously or not. In order to be able
to make this better, we now peek into the passed arrow function and
check for a synchronous query usage. If so, we set the query timing to
static and mark it as ambiguous. This ensures that the usage strategy is
less "magical" and more correct with third-party code.

Additionally since functions like setTimeout are not analyzable but known
to be asynchronous, there is a hard-coded list of known functions which
shouldn't be marked as ambiguous.

Resolves FW-1214. As planned within https://hackmd.io/hPiLWpPlQ4uynC1luIBdfQ

@CaerusKaru
Copy link
Member

left a comment

LGTM

@kara

kara approved these changes May 8, 2019

Copy link
Contributor

left a comment

LGTM

@kara kara removed their assignment May 8, 2019

@kara

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

@devversion Think this should be fix(core) as well in the commit message

@kara

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

@devversion devversion force-pushed the devversion:static-query-ambiguous-usage branch from c4c9448 to 790f933 May 9, 2019

@devversion

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

@kara I'm not sure if it's really considered a fix, but I agree that it's not a refactor either. Renamed to fix.

@devversion devversion force-pushed the devversion:static-query-ambiguous-usage branch from 790f933 to d2dc2f8 May 9, 2019

@devversion devversion force-pushed the devversion:static-query-ambiguous-usage branch from d2dc2f8 to eabc4ad May 9, 2019

fix(core): static-query usage migration strategy should detect ambigu…
…ous query usage

Currently we always just set the timing to `false` if we aren't
able to analyze a given call expression or new expression. e.g.

```ts
ngOnInit() {
  thirdPartyCallSync(() => this.query.doSomething())
}
```

In that case the `thirdPartyCallSync` function comes from the `node_modules`
and is only defined through types while there is no code for the
actual function logic that can be analyzed. This makes it impossible
to tell whether the given call expression actually causes the specified
arrow function to be executed synchronously or not. In order to be able
to make this better, we now peek into the passed arrow function and
check for a synchronous query usage. If so, we set the query timing to
static and mark it as ambiguous. This ensures that the usage strategy is
less "magical" and more correct with third-party code.

Additionally since functions like `setTimeout` are not analyzable but known
to be asynchronous, there is a hard-coded list of known functions which
shouldn't be marked as ambiguous.

Resolves FW-1214. As planned within https://hackmd.io/hPiLWpPlQ4uynC1luIBdfQ

@devversion devversion force-pushed the devversion:static-query-ambiguous-usage branch from eabc4ad to 5065c25 May 9, 2019

alxhub added a commit that referenced this pull request May 9, 2019

fix(core): static-query usage migration strategy should detect ambigu…
…ous query usage (#30215)

Currently we always just set the timing to `false` if we aren't
able to analyze a given call expression or new expression. e.g.

```ts
ngOnInit() {
  thirdPartyCallSync(() => this.query.doSomething())
}
```

In that case the `thirdPartyCallSync` function comes from the `node_modules`
and is only defined through types while there is no code for the
actual function logic that can be analyzed. This makes it impossible
to tell whether the given call expression actually causes the specified
arrow function to be executed synchronously or not. In order to be able
to make this better, we now peek into the passed arrow function and
check for a synchronous query usage. If so, we set the query timing to
static and mark it as ambiguous. This ensures that the usage strategy is
less "magical" and more correct with third-party code.

Additionally since functions like `setTimeout` are not analyzable but known
to be asynchronous, there is a hard-coded list of known functions which
shouldn't be marked as ambiguous.

Resolves FW-1214. As planned within https://hackmd.io/hPiLWpPlQ4uynC1luIBdfQ

PR Close #30215

@alxhub alxhub closed this in 8d3365e May 9, 2019

BioPhoton added a commit to BioPhoton/angular that referenced this pull request May 21, 2019

fix(core): static-query usage migration strategy should detect ambigu…
…ous query usage (angular#30215)

Currently we always just set the timing to `false` if we aren't
able to analyze a given call expression or new expression. e.g.

```ts
ngOnInit() {
  thirdPartyCallSync(() => this.query.doSomething())
}
```

In that case the `thirdPartyCallSync` function comes from the `node_modules`
and is only defined through types while there is no code for the
actual function logic that can be analyzed. This makes it impossible
to tell whether the given call expression actually causes the specified
arrow function to be executed synchronously or not. In order to be able
to make this better, we now peek into the passed arrow function and
check for a synchronous query usage. If so, we set the query timing to
static and mark it as ambiguous. This ensures that the usage strategy is
less "magical" and more correct with third-party code.

Additionally since functions like `setTimeout` are not analyzable but known
to be asynchronous, there is a hard-coded list of known functions which
shouldn't be marked as ambiguous.

Resolves FW-1214. As planned within https://hackmd.io/hPiLWpPlQ4uynC1luIBdfQ

PR Close angular#30215
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.