-
-
Notifications
You must be signed in to change notification settings - Fork 115
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
Faster fragment parsing #334
Conversation
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. |
@StorytellerCZ Happy to do so - what should the format be, should I add a new heading |
@znewsham Yes, follow the format of the previous releases. |
@StorytellerCZ done |
Hi @znewsham thanks for this. Could you add some tests for these changes? |
Just a comment in perspective - it has been requested some times to remove jQuery. The Edit: The function does not look that complex on a first glimpse, maybe we can extract it into the package soon? |
Agreed with @jankapunkt if we could do this without jQuery that would be awesome! |
@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 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! |
I will not remove jQuery yet but I can fix the merge conflicts and add some tests so the pr can be merged soon 🙂 |
@jankapunkt please proceed. |
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? |
@jankapunkt assuming you've cloned the package into a local app's
should trigger the new behaviour (e.g., any element where you don't use |
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's
parseHTML` 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
fromDOMBackend.getContext
- but this is "more correct". The code is taken fromjq.parseHTML
I'm wondering if anyone can see any issues with this? Possibly if rendering into an iframe?