Skip to content

Add the script tag after-render#48

Open
stefanpenner wants to merge 2 commits intomasterfrom
add-new-code-after-render
Open

Add the script tag after-render#48
stefanpenner wants to merge 2 commits intomasterfrom
add-new-code-after-render

Conversation

@stefanpenner
Copy link
Contributor

@stefanpenner stefanpenner commented May 26, 2017 •
edited
Loading

Kinda strange to add to the DOM during just any period, we likely want this to always happen when stuff has settled down.

In-theory this could be made pluggable, allowing an application to provide an alternative schedular.

import { setScriptScheduler } from '...';

setScriptScheduler(function(cb) {
  schedular.scheduleWork('afterContentPaint', cb);
});

cc @scalvert

trentmwillis
trentmwillis suggested changes May 27, 2017
View reviewed changes
Copy link
Contributor

@trentmwillis trentmwillis left a comment

Choose a reason for hiding this comment

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

I wouldn't say it is "kinda strange", it is just the simple approach. The current approach doesn't cause any abnormal behavior, but it is likely non-optimal.

Should we do the same thing for CSS loader? It likely isn't as big of a deal as loading JS mid-run-loop, but seems like it may still be beneficial.

addon/loaders/js.js
document.head.appendChild(script);
document.head.appendChild(script);
} catch(e) {
reject(e);
Copy link
Contributor

@trentmwillis trentmwillis May 26, 2017

Choose a reason for hiding this comment

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

What error would we expect to happen here?

Copy link
Contributor Author

@stefanpenner stefanpenner May 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

none, this is to handle the exceptional cases.

Copy link
Contributor Author

@stefanpenner stefanpenner May 30, 2017

Choose a reason for hiding this comment

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

This also maintains existing behavior. Currently if any of those lines error, the promise constructor will catch them. As this PR moves them to be async, we must manage that ourselves.

addon/loaders/js.js Outdated
}

const script = createLoadElement('script', resolve, reject);
Ember.run.schedule('afterRender', () => {
Copy link
Contributor

@trentmwillis trentmwillis May 27, 2017

Choose a reason for hiding this comment

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

We should add a comment as to why this afterRender is present.

stefanpenner and others added 2 commits August 11, 2017 08:58
@stefanpenner
Add the script tag after-render
234caeb
Fixup test assertions.
96f90e8 
We cannot simply return the promise in the failure scenarios (since
returning a rejected promise will fail the test).  I have added
back a small assertion confirming the rejections in cases we _know_
should reject.

The CSS load failure scenario is a bit annoying/odd because we cannot
guarantee that we fail properly when the asset cannot be loaded (due
to lack of browser support for `onload`/`onerror` callbacks with the
`<link>` element).
@rwjblue rwjblue force-pushed the add-new-code-after-render branch from 9a84986 to 96f90e8 Compare August 11, 2017 13:09
@rwjblue
Copy link
Member

rwjblue commented Aug 11, 2017

Rebased this on top of latest master, and fixed the two failing assertions (both around service.loadAsset() rejecting).

@stefanpenner / @trentmwillis - This is ready for another look.

@rwjblue rwjblue requested a review from trentmwillis August 11, 2017 13:10
trentmwillis
trentmwillis approved these changes Aug 15, 2017
View reviewed changes
Copy link
Contributor

@trentmwillis trentmwillis left a comment

Choose a reason for hiding this comment

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

One small nitpick, but otherwise looks good to me!

addon/loaders/css.js
}

// Try using the default onload/onerror handlers...
const link = createLoadElement('link', resolve, reject);
Copy link
Contributor

@trentmwillis trentmwillis Aug 11, 2017

Choose a reason for hiding this comment

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

Why move this declaration outside the block where it is initialized?

@rwjblue
Copy link
Member

rwjblue commented Mar 19, 2018

@stefanpenner - Mind rebasing?

@villander
Copy link
Contributor

villander commented Jan 9, 2019

@stefanpenner mind rebasing? please

@stefanpenner
Copy link
Contributor Author

stefanpenner commented Jan 9, 2019

sure

@villander
Copy link
Contributor

villander commented Jan 23, 2019

@stefanpenner I'd love to see this merge, you want some help?

@stefanpenner
Copy link
Contributor Author

stefanpenner commented Mar 12, 2019

@villander yes please

@villander
Copy link
Contributor

villander commented Apr 16, 2019

@stefanpenner conflicts solved by #81

@villander villander mentioned this pull request May 10, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@trentmwillis trentmwillis trentmwillis approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@stefanpenner @rwjblue @villander @trentmwillis