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

Ivy resource loader #24637

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@mhevery
Member

mhevery commented Jun 23, 2018

feat(ivy): Support resource resolution in JIT mode.

Used to resolve resource URLs on @Component when used with JIT compilation.

@Component({
  selector: 'my-comp',
  templateUrl: 'my-comp.html', // This requires asynchronous resolution
})
class MyComponnent{
}

// Calling `renderComponent` will fail because `MyComponent`'s `@Compenent.templateUrl`
// needs to be resolved because `renderComponent` is synchronous process.
// renderComponent(MyComponent);

// Calling `resolveComponentResources` will resolve `@Compenent.templateUrl` into
// `@Compenent.template`, which would allow `renderComponent` to proceed in synchronous manner.
// Use browser's `fetch` function as the default resource resolution strategy.
resolveComponentResources(fetch).then(() => {
  // After resolution all URLs have been converted into strings.
  renderComponent(MyComponent);
});

@googlebot googlebot added the cla: yes label Jun 23, 2018

@mhevery mhevery requested a review from alxhub Jun 23, 2018

@alfaproject

This comment has been minimized.

Contributor

alfaproject commented Jun 23, 2018

Any reason not to use Angular's own HTTP implementation by default?

@Toxicable

This comment has been minimized.

Contributor

Toxicable commented Jun 24, 2018

@alfaproject That would mean that Core would depend on Common already depends on Core, this would be a cyclic dependancy.

@alxhub

alxhub approved these changes Jun 26, 2018

Otherwise LGTM

* URL. Browser's `fetch` method is a good default implementation.
*/
export function resolveComponentResources(
resourceResolver: (url: string) => Promise<string|Response>): Promise<null> {

This comment has been minimized.

@alxhub

alxhub Jun 26, 2018

Contributor

Angular does not currently have a dependency on fetch, even via typings. It's not supported, for example, in IE < Edge.

What about removing Response from our public API typings and instead using the signature: (url: string) => Promise<string | { text(): Promise<string> }>. This fits fetch() without introducing a dependency on DOM typings.

Also, Promise<void> is the typical way of expressing that a function returns a Promise that has no meaning other than its resolution.

This comment has been minimized.

@mhevery

mhevery Jun 26, 2018

Member

Good idea, done.

@ngbot

This comment has been minimized.

ngbot bot commented Jun 26, 2018

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure missing required status "code-review/pullapprove"
    pending status "ci/circleci: lint" is pending
    pending status "ci/circleci: build-packages-dist" is pending
    pending status "ci/circleci: test_ivy_aot" is pending
    pending status "google3" is pending
    pending status "ci/circleci: test_ivy_jit" is pending
    pending status "ci/circleci: test" is pending
    pending status "continuous-integration/travis-ci/pr" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

styleUrls && styleUrls.forEach((styleUrl) => {
cachedResourceResolve(styleUrl).then((style) => {
if (!component.styles) component.styles = [];
component.styles.push(style);

This comment has been minimized.

@trotyl

trotyl Jun 27, 2018

Contributor

This would reorder the styles according to response finish time rather than preserve the declared order, right?

This comment has been minimized.

@mhevery

mhevery Jun 27, 2018

Member

Good catch, fixed and added a test.

@@ -93,6 +93,27 @@ Did you run and wait for 'resolveComponentResources()'?`.trim());
expect(resourceFetchCount).toBe(1);
}));
fit('should keep order even if the resolution is out of order', await(async() => {

This comment has been minimized.

@alfaproject

alfaproject Jun 27, 2018

Contributor

fit

P.S.: Do you need await? Thought jasmine supports promises now

@mary-poppins

This comment has been minimized.

mary-poppins commented Jun 28, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Jun 28, 2018

You can preview 17c48ca at https://pr24637-17c48ca.ngbuilds.io/.

mhevery added some commits Jun 23, 2018

feat(core): add support for using async/await with Jasmine
Example:
```
it('...', await(async() => {
  doSomething();
  await asyncFn();
  doSomethingAfter();
}));
```
feat(ivy): Support resource resolution in JIT mode.
Used to resolve resource URLs on `@component` when used with JIT compilation.

```
@component({
  selector: 'my-comp',
  templateUrl: 'my-comp.html', // This requires asynchronous resolution
})
class MyComponnent{
}

// Calling `renderComponent` will fail because `MyComponent`'s `@Compenent.templateUrl`
// needs to be resolved because `renderComponent` is synchronous process.
// renderComponent(MyComponent);

// Calling `resolveComponentResources` will resolve `@Compenent.templateUrl` into
// `@Compenent.template`, which would allow `renderComponent` to proceed in synchronous manner.
// Use browser's `fetch` function as the default resource resolution strategy.
resolveComponentResources(fetch).then(() => {
  // After resolution all URLs have been converted into strings.
  renderComponent(MyComponent);
});

```
@mary-poppins

This comment has been minimized.

mary-poppins commented Jun 28, 2018

@mhevery mhevery closed this in 71100e6 Jun 28, 2018

mhevery added a commit that referenced this pull request Jun 28, 2018

feat(ivy): Support resource resolution in JIT mode. (#24637)
Used to resolve resource URLs on `@component` when used with JIT compilation.

```
@component({
  selector: 'my-comp',
  templateUrl: 'my-comp.html', // This requires asynchronous resolution
})
class MyComponnent{
}

// Calling `renderComponent` will fail because `MyComponent`'s `@Compenent.templateUrl`
// needs to be resolved because `renderComponent` is synchronous process.
// renderComponent(MyComponent);

// Calling `resolveComponentResources` will resolve `@Compenent.templateUrl` into
// `@Compenent.template`, which would allow `renderComponent` to proceed in synchronous manner.
// Use browser's `fetch` function as the default resource resolution strategy.
resolveComponentResources(fetch).then(() => {
  // After resolution all URLs have been converted into strings.
  renderComponent(MyComponent);
});

```

PR Close #24637
* ```
*
*/
export function jasmineAwait(fn: () => Promise<any>):

This comment has been minimized.

@alfaproject

alfaproject Jun 29, 2018

Contributor

FYI Jasmine 2.8 supports this natively

This comment has been minimized.

@mhevery

This comment has been minimized.

@dman777

dman777 Jul 26, 2018

Is the async/await support in jasmine(exposed in this PR) only for the Ivy compiler? Or does this support also apply to the current AOT/JIT compilers? Wasn't sure why this was under the Ivy context.

This comment has been minimized.

@JoostK

JoostK Jul 26, 2018

Contributor

@dman777 This API change has been reverted yesterday in e1c6fd5 so it doesn't really matter ;)

This comment has been minimized.

@dman777

dman777 Jul 26, 2018

Oh, the readme for 6.1 shows otherwise. Is the readme in error?

This comment has been minimized.

@JoostK

JoostK Jul 26, 2018

Contributor

@dman777 Oh man I was worried this ended up in 6.1 for a second. It has been reverted just before 6.1 was cut, so it has NOT been released. The changelog is generated automatically based on the conventional commits format, where apparently the revert commit hasn't cancelled the creation commit. So yes, the changelog is in error.

This comment has been minimized.

@dman777

dman777 Jul 26, 2018

Ok, thanks for the heads up. I hope this goes back in soon. Now that Jasmine is testing the DOM there can be race conditions that are hard to test for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment