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

Faster fragment parsing #334

Merged
merged 5 commits into from
Mar 21, 2023

Conversation

znewsham
Copy link
Contributor

@znewsham znewsham commented May 8, 2021

Happy to see Blaze getting some Love - I've been using this change in production for a good long while, figured it was maybe time to merge.

Whenever you render raw HTML in {{{}}} and whenever you render any static HTML that has no {{}} in it (e.g., <li>Test</li>) jquery'sparseHTML` is called, which generates a new document context. This causes some pieces of jquery to try and figure out the compatibility (IE/ Safari quirks, etc) of the current document EVERY time an element like this is rendered.

I'm 99% sure we could just return document from DOMBackend.getContext - but this is "more correct". The code is taken from jq.parseHTML

I'm wondering if anyone can see any issues with this? Possibly if rendering into an iframe?

@StorytellerCZ
Copy link
Collaborator

I have nothing to add from myself (which counts little), but it would be nice if you could add entry to history that lets people know about the significance of the change.

@znewsham
Copy link
Contributor Author

@StorytellerCZ Happy to do so - what should the format be, should I add a new heading v.2.4.1, 2021-May-12 and list it there?

@StorytellerCZ
Copy link
Collaborator

@znewsham Yes, follow the format of the previous releases.

@znewsham
Copy link
Contributor Author

@StorytellerCZ done

@filipenevola
Copy link
Collaborator

Hi @znewsham thanks for this.

Could you add some tests for these changes?

@filipenevola filipenevola added the pending-tests Tests are not passing, stuck or we need new tests label May 12, 2021
@jankapunkt
Copy link
Collaborator

jankapunkt commented Jun 7, 2021

Just a comment in perspective - it has been requested some times to remove jQuery. The parseHTML is currently a crucial part of Blaze but relies on jQuery. Nothing wrong with this PR, I actually am grateful for this one ❤️

Edit: The function does not look that complex on a first glimpse, maybe we can extract it into the package soon?

@StorytellerCZ
Copy link
Collaborator

Agreed with @jankapunkt if we could do this without jQuery that would be awesome!

@jankapunkt
Copy link
Collaborator

@znewsham is there anything you need in order to improve on this? If you add me to your repo I can help to continue this PR

@jankapunkt jankapunkt mentioned this pull request Oct 2, 2021
@znewsham
Copy link
Contributor Author

znewsham commented Oct 2, 2021

@jankapunkt Unfortunately I'm completely swamped at the moment, and removing jQuery has no benefits to me since I use jQuery anyway :D so this isn't a high priority for me.

Thanks for offering to help! I added you to the repo!

@jankapunkt
Copy link
Collaborator

I will not remove jQuery yet but I can fix the merge conflicts and add some tests so the pr can be merged soon 🙂

@StorytellerCZ
Copy link
Collaborator

@jankapunkt please proceed.

@StorytellerCZ StorytellerCZ added this to the Blaze 3.0 milestone Jan 1, 2022
packages/blaze/dombackend.js Show resolved Hide resolved
packages/blaze/dombackend.js Show resolved Hide resolved
@StorytellerCZ StorytellerCZ changed the base branch from master to release-3.0 April 15, 2022 13:49
@jankapunkt
Copy link
Collaborator

Hey @znewsham I am getting back at this one, finally. What would I need to do in order to tests this one in a running app using Blaze?

@znewsham
Copy link
Contributor Author

@jankapunkt assuming you've cloned the package into a local app's packages directory for testing, rendering raw HTML from a helper:

//js
Template.whatever.helpers({rawHtml() { return "<span>Hello</span>";}})
//html
<template name="whatever">
{{{rawHtml}}}
<span>Hello</span>
</template>

should trigger the new behaviour (e.g., any element where you don't use {{}} in the tag)

@jankapunkt jankapunkt merged commit 938361f into meteor:release-3.0 Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending-tests Tests are not passing, stuck or we need new tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants