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

feat: enable load callbacks #414

Merged
merged 24 commits into from
Jul 24, 2019
Merged

feat: enable load callbacks #414

merged 24 commits into from
Jul 24, 2019

Conversation

pimlie
Copy link
Collaborator

@pimlie pimlie commented Jul 18, 2019

Resolves: #240

This pr adds the possibility for adding callbacks which are called when eg the script has been loaded. This makes it possible to chain scripts which are depending on each other.

Client side implementation

When the element is created the onload attribute is just directly set

Server side implementation

  • Elements need to have a ${tagIDKeyName} set (vmid / hid)
  • When a tag has a callback, an onload attribute is added onload="this.__vm_l=1"
  • On hydration, VueMeta will check for the ${ssrAttribute} attribute
  • When this exists, the load callbacks are added
  • The corresponding callback for an element is resolved using the tagIDKeyName
  • On existing elements an load event listener is directly added
  • When the DOM is not fully loaded, an event listener is added onReadyStateChange which adds callback to available elements
  • Once the load callback is triggered for a tag, the onload attribute will be removed from the element

VueMeta uses the following three props directly on the element for hydration/ load callback control:

  • __vm_cb whether the load callback has been called
  • __vm_l set by onload attribute, whether the element was loaded
  • __vm_ev whether the event listener was added or not

skip attribute

I have added a skip attribute for tag definitions in metaInfo so its easier to do reactive chaining (see async-callback example)

Potential caveats

  • Callbacks on tags which are loaded before the main app will probably not trigger not anymore due to onload attribute which is always added

ToDo

  • add tests

@pimlie pimlie requested review from atinux and TheAlexLichter July 18, 2019 11:48
atinux
atinux previously approved these changes Jul 18, 2019
Copy link
Member

@atinux atinux left a comment

Choose a reason for hiding this comment

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

LGTM :)

@galvez
Copy link

galvez commented Jul 18, 2019

daaaamn

You've been busy :D

TheAlexLichter
TheAlexLichter previously approved these changes Jul 18, 2019
Copy link
Member

@TheAlexLichter TheAlexLichter left a comment

Choose a reason for hiding this comment

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

LGTM so far. Only 2 small things

examples/async-callback/app.js Outdated Show resolved Hide resolved
examples/async-callback/app.js Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 22, 2019

Codecov Report

Merging #414 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #414   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          31     33    +2     
  Lines         449    511   +62     
  Branches      127    145   +18     
=====================================
+ Hits          449    511   +62
Impacted Files Coverage Δ
src/client/$meta.js 100% <ø> (ø) ⬆️
src/client/updateClientMetaInfo.js 100% <100%> (ø) ⬆️
src/server/generators/tag.js 100% <100%> (ø) ⬆️
src/shared/constants.js 100% <100%> (ø) ⬆️
src/client/updaters/tag.js 100% <100%> (ø) ⬆️
src/shared/merge.js 100% <100%> (ø) ⬆️
src/shared/log.js 100% <100%> (ø)
src/client/load.js 100% <100%> (ø)
src/shared/mixin.js 100% <100%> (ø) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05163a7...9e50b87. Read the comment docs.

@pimlie pimlie requested review from TheAlexLichter and atinux July 22, 2019 14:26
@pimlie
Copy link
Collaborator Author

pimlie commented Jul 22, 2019

Due to issues in IE9, I've opted for a more basic approach. Its quite ugly if you ask me, but adding the onload attribute on ssr makes sure that we know the element has been loaded also when the loading takes place before we can add a load event listener.

I've also removed the MutationObserver and just try to add callbacks on every document.readyState change.

Have to fix the tests again once you approve.

* __vm_l: set by onload attribute, whether the element was loaded
* __vm_ev: whether the event listener was added or not
*/
if (element.__vm_cb) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've could have made this an object like __vm = {} instead but as __vm_l needs to be set from the onload attribute and using an object there is too complicated, this approach of separate variables seemed ok to me.

@nuxt nuxt deleted a comment from codecov bot Jul 22, 2019
@pimlie pimlie marked this pull request as ready for review July 22, 2019 14:45
@pimlie pimlie dismissed stale reviews from TheAlexLichter and atinux July 22, 2019 14:46

major refactor

@pimlie pimlie merged commit fc71e1f into master Jul 24, 2019
@pimlie pimlie deleted the feat-load-callback branch July 24, 2019 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return a Promise for async scripts?
4 participants