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

Add caching + supabase backend to snippet #67

Merged
merged 4 commits into from
Apr 28, 2021
Merged

Add caching + supabase backend to snippet #67

merged 4 commits into from
Apr 28, 2021

Conversation

econti
Copy link
Collaborator

@econti econti commented Apr 28, 2021

No description provided.

@econti econti force-pushed the cache_supa branch 2 times, most recently from 1ccd021 to 7028e73 Compare April 28, 2021 17:06
Comment on lines +130 to +133
document.addEventListener('DOMContentLoaded', (event) => {
const changesAppliedFromCache = refineryAPI.applyChangesFromCache();
refineryAPI.applyChangesFromProd(changesAppliedFromCache);
})
Copy link
Collaborator Author

@econti econti Apr 28, 2021

Choose a reason for hiding this comment

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

Perhaps we can do something smarter here. Document.readyState seemed to be slower from some local tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yea i think both would have to be done for any potential benefit, e.g.

const changesAppliedFromCache = refineryAPI.applyChangesFromCache()
if (document.readyState === "complete" || document.readyState === "interactive") {
  refineryAPI.applyChangesFromProd(changesAppliedFromCache)
} else {
  document.addEventListener("DOMContentLoaded", () => refineryAPI.applyChangesFromProd(changesAppliedFromCache))
}

might be slightly faster if user ends up putting our script at the end instead of beginning and document is already loaded? not sure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh smart! that's a good idea. i'll add it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm, doesn't seem to work.

can't do this first because the DOM isn't ready yet:

const changesAppliedFromCache = refineryAPI.applyChangesFromCache()

and this doesn't apply any changes for some reason

let refineryAPI = new refinery.RefineryAPI();
if (document.readyState === "complete" || document.readyState === "interactive") {
  refineryAPI.applyChangesFromCache()
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh got it, nm!

Comment on lines +117 to +122
if (!(changesAppliedFromCache.has(change.id))) {
// the change hasn't been applied from the cache so we must apply it
// this works as long as we don't allow edits to changes (we currently don't)
// once we do, then we will need to make a check if the change has changed -_-
self.applyChange(change);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

or we could always apply the change no matter what - if they match, nothing happens. simpler code and handles both scenarios

Copy link
Collaborator Author

@econti econti Apr 28, 2021

Choose a reason for hiding this comment

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

Would nothing happen though? I was trying to guard against a flicker (even if the same text will be filled, just the filling itself may cause a flicker, but not entirely sure)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, a very, very, very small benefit of doing it this way is that the prod changes will get applied faster.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would nothing happen though?
pretty sure, at no time does innerHTML = null.
will get applied faster.
not noticably.

its not a big deal if you dont think its worth it.

@econti econti merged commit 31c706f into master Apr 28, 2021
@econti econti deleted the cache_supa branch April 28, 2021 17:28
Copy link
Collaborator

@lgvital lgvital left a comment

Choose a reason for hiding this comment

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

great stuff all around @econti !

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.

4 participants