Skip to content

Ivy resource loader - #24637

Closed
mhevery wants to merge 2 commits into
angular:masterfrom
mhevery:ivy_resource_loader
Closed

mhevery wants to merge 2 commits into
angular:masterfrom
mhevery:ivy_resource_loader

Conversation

@mhevery

@mhevery mhevery commented Jun 23, 2018

Copy link
Copy Markdown
Contributor

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);
});

@mhevery
mhevery requested a review from alxhub June 23, 2018 02:06
@alfaproject

Copy link
Copy Markdown
Contributor

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

@Toxicable

Copy link
Copy Markdown

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

@alxhub alxhub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Otherwise LGTM

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea, done.

@mhevery mhevery added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release labels Jun 26, 2018
@ngbot

ngbot Bot commented Jun 26, 2018

Copy link
Copy Markdown

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.

@trotyl trotyl Jun 27, 2018 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, fixed and added a test.

@alfaproject alfaproject Jun 27, 2018 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

fit

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

@mary-poppins

Copy link
Copy Markdown

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

@mary-poppins

Copy link
Copy Markdown

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

mhevery added 2 commits June 28, 2018 13:56
Example:
```
it('...', await(async() => {
  doSomething();
  await asyncFn();
  doSomethingAfter();
}));
```
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);
});

```
@mhevery
mhevery force-pushed the ivy_resource_loader branch from 17c48ca to 7ec241b Compare June 28, 2018 17:58
@mary-poppins

Copy link
Copy Markdown

You can preview 7ec241b at https://pr24637-7ec241b.ngbuilds.io/.

@mhevery mhevery closed this in 71100e6 Jun 28, 2018
mhevery added a commit that referenced this pull request Jun 28, 2018
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>):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FYI Jasmine 2.8 supports this natively

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@dman777 dman777 Jul 26, 2018 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

@angular-automatic-lock-bot

Copy link
Copy Markdown

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 Sep 13, 2019
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 cla: yes target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants