GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Ivy resource loader by mhevery · Pull Request #24637 · angular/angular

Conversation

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

googlebot added the cla: yes label

Jun 23, 2018

mhevery requested a review from alxhub

June 23, 2018 02:06

alfaproject commented Jun 23, 2018

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Toxicable commented Jun 24, 2018

Copy link

Copy Markdown

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

spock123, alfaproject, sandangel, and trotyl reacted with thumbs up emoji

All reactions

👍 4 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

alxhub approved these changes

Jun 26, 2018

View reviewed changes

alxhub left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Otherwise LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

packages/core/src/metadata/resource_loading.ts Outdated

alxhub Jun 26, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mhevery Jun 26, 2018

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Good idea, done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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 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:
     missing required status "code-review/pullapprove"
     status "ci/circleci: lint" is pending
     status "ci/circleci: build-packages-dist" is pending
     status "ci/circleci: test_ivy_aot" is pending
     status "google3" is pending
     status "ci/circleci: test_ivy_jit" is pending
     status "ci/circleci: test" is 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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

trotyl reviewed

Jun 27, 2018

View reviewed changes

Comment thread

packages/core/src/metadata/resource_loading.ts Outdated

trotyl Jun 27, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pankajparkar reacted with thumbs up emoji

All reactions

👍 1 reaction

mhevery Jun 27, 2018

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

alfaproject reviewed

Jun 27, 2018

View reviewed changes

Comment thread

packages/core/test/metadata/resource_loading_spec.ts Outdated

alfaproject Jun 27, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mary-poppins commented Jun 28, 2018

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mary-poppins commented Jun 28, 2018

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mhevery added 2 commits

June 28, 2018 13:56


          feat(core): add support for using async/await with Jasmine

f6829ab

Example:
```
it('...', await(async() => {
  doSomething();
  await asyncFn();
  doSomethingAfter();
}));
```


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

7ec241b

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 force-pushed the ivy_resource_loader branch from 17c48ca to 7ec241b Compare

June 28, 2018 17:58

mary-poppins commented Jun 28, 2018

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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)

0ede987

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

alfaproject reviewed

Jun 29, 2018

View reviewed changes

Comment thread

packages/core/testing/src/jasmine_await.ts

		* ```
		*
		*/
		export function jasmineAwait(fn: () => Promise<any>):

alfaproject Jun 29, 2018

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mhevery Jul 2, 2018

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

#24733

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

dman777 Jul 26, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

JoostK Jul 26, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

dman777 Jul 26, 2018

Copy link

Copy Markdown

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

JoostK Jul 26, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

dman777 Jul 26, 2018

Copy link

Copy Markdown

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

angular-automatic-lock-bot Bot commented Sep 13, 2019

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

angular-automatic-lock-bot Bot locked and limited conversation to collaborators

Sep 13, 2019

Reviewers

JoostK

JoostK left review comments

alxhub

alxhub approved these changes

+3 more reviewers

alfaproject

alfaproject left review comments

dman777

dman777 left review comments

trotyl

trotyl left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

mhevery commented Jun 23, 2018

Uh oh!

alfaproject commented Jun 23, 2018

Uh oh!

Toxicable commented Jun 24, 2018

Uh oh!

alxhub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngbot Bot commented Jun 26, 2018

Uh oh!

trotyl Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alfaproject Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Jun 28, 2018

Uh oh!

mary-poppins commented Jun 28, 2018

Uh oh!

mary-poppins commented Jun 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dman777 Jul 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot Bot commented Sep 13, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

trotyl Jun 27, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

alfaproject Jun 27, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

dman777 Jul 26, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.