-
-
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
Release 2.6.1 #371
Release 2.6.1 #371
Conversation
Prevent firing callbacks until members are destroyed
blaze@2.6.1-rc.0 is ready to be tested. |
Remove tests for removed API in spacebars, fix #369
2.6.1-rc.1 released. |
I think the rc has been out enough, can we release it? I'd like to fix the Travis error in |
@harryadel I found that #366 is not a full solution to the problem, basically this was an XY problem and I will add a new fix tomorrow. |
Blaze.remove should destroy view before detaching
@jankapunkt anythings else that should be added? |
Please let me know when it's ready, and I'll release the official version. 🙌 |
Hi guys. Sorry for taking so long to give some feedback on this. I tried it in staging for ages and nobody noticed anything wrong. We launched it last night to production and then discovered a bug this morning unfortunately. I'll try and see if I can debug a little or give you some clear picture of why/how it happens but just wanted to give you a headsup that there may be an issue. It could just be a peculiarity of how we're using it. Basically I have a template something like {{#if someCondition}} someCondition checks a query param which can be toggled. What I'm seeing it that the selects are not getting destroyed when I flick between them and just accumulate. |
@lynchem Does this problem still persist? If so, can you please create a reproduction repo so we can look further into it? |
Sorry for the delay @harryadel and for holding this up. I think the PR is good and the issue I'm having it purely down to the weird control's behaviour. If I switch it out for a regular select or even a datepicker it works fine, so clearly something with how this particular control works. |
Now it outputs a more descriptive version: `${event} event triggerd with ${selector} on ${templateName} but associated view is not be found. Make sure the event doesn't destroy the view.` Fixes #213
…fix #213 Merge pull request #376 from harryadel/view-must-be-attached-error Thanks to @harryadel
…#374 Add a test for #374 Merge pull request #377 from harryadel/add-tests thanks to @harryadel
Info: I merged #374 and #377 now what I want to do is, as a last step before publishing, is to checkout this branch with one or two projects of us. Both have complex setups with Blaze and I'd like to see, if we are good now with everything. I will also do a last check on documentation of recent changes to make it easier for those who are not so closely following the issues and PRs here. |
@jankapunkt let us know once you are done. @denihs I think we can have one more RC release here. |
It's done! blaze@2.6.1-rc.2 is out. |
I am currently using this in our staging environment and we are currently doing lots of UX testing internally so if there are certain Blaze issues it will pop out. I will give feedback at the end of the week. |
@jankapunkt @denihs if there are no issue then we can release next week. |
@StorytellerCZ from end that would work. There is actually one domrange issue that I found (unrelated to any of the existing ones) but I still have to figure out, whether this is a problem of my implementation. I will definitely make a summary on Friday about my testing. |
Okay @denihs @StorytellerCZ @harryadel I did used the rc release in our staging env and this is what I covered:
Error handling and search for DOMRange issuesI then did setup another project to check through all the issues with errors during any lifecycle method. For reproduction purposes I did the following: $ meteor create testproject --blaze
$ cd testproject && meteor Then manually added Then replaced <head>
<title>testproject</title>
</head>
<body>
<select class="select-template">
<option>none</option>
{{#each option in options}}
<option value="template{{option}}">Template {{option}}</option>
{{/each}}
</select>
{{#if show}}
{{> Template.dynamic template=show }}
{{/if}}
</body>
<template name="templateA">TemplateA</template>
<template name="templateB">templateB</template>
<template name="templateC">templateC {{raise}}</template>
<template name="templateD">templateD <button class="raise-btn">raise</button></template>
<template name="templateE">TemplateE
<div id="renderTarget"></div>
<button class="render-btn">render</button>
<button class="remove-btn">remove</button>
</template>
<template name="templateF">templateF</template>
<template name="templateEInner">Inner Template with Blaze.render</template> and the following code in import { Template } from 'meteor/templating'
import { ReactiveVar } from 'meteor/reactive-var'
import './main.html'
import { Blaze } from 'meteor/blaze'
Template.body.onCreated(function () {
this.show = new ReactiveVar()
})
Template.body.helpers({
show () {
return Template.instance().show.get()
},
options () {
return [...'ABCDEF']
}
})
Template.body.events({
'click .remove-btn' () {
Blaze.remove(renderedView)
},
'change .select-template' (event, templateInstance) {
const value = templateInstance.$(event.currentTarget).val()
templateInstance.show.set(value)
}
})
Template.templateA.onCreated(function () {
throw new Error('Error-1')
})
Template.templateB.onRendered(function () {
throw new Error('Error-2')
})
Template.templateC.helpers({
raise () {
throw new Error('Error-3')
}
})
Template.templateD.events({
'click .raise-btn' () {
throw new Error('Error-4')
}
})
let renderedView
Template.templateE.events({
'click .render-btn' (event, templateInstance) {
const parent = templateInstance.$('#renderTarget').get(0)
renderedView = Blaze.render(Template.templateEInner, parent)
},
'click .remove-btn' (event, templatInstance) {
Blaze.remove(renderedView)
}
})
Template.templateEInner.onDestroyed(function () {
throw new Error('Error-5')
})
Template.templateF.onDestroyed(function () {
throw new Error('Error-6')
}) Here Are my results on Created - Dev
This caused a follow-up error, which always occurs when Template A is removed:
This also causes no other Template to be drawn afterwards, although they should! Should I take hands on this issue? I think this is crucial to a proper user experience. on Created - ProdIn production it's worse since nobody knows where this comes from:
And the DOMRange error is completely obfuscated:
This is something we definitely need to tackle! I think this will be much better, when we integrate #368 onRendered - dev
onRendered - prod
Helpers - dev
Helpers - prod
Events - dev
Events - prod
onDestroyed and manual Blaze.remove - dev
onDestroyed and manual Blaze.remove - prod
onDestroyed after automatic remove - dev
onDestroyed after automatic remove - prod
My opinion on that
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I get approvals from you @StorytellerCZ and @harryadel I will merge and update the HISTORY.md
and package numbers and then it's finally ready to be released
@@ -1,3 +1,9 @@ | |||
## v2.6.1, 2022-April-XX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor thing but we need to update this to the date of relase
So I tried to dig into the issue we were having a little more. I believe the problem stems from a jquery control modifying the DOM. Our template is one we use for filters in a number of places. The reason we noticed the issue here is because on this particular page you can toggle from two sets of pre-defined values so it destroys and recreates the template. The template looks like this
In our onRendered we do
In our onDestroyed we call Has something changed with how we track what we need to remove? I'm not sure if the previous behaviour was simply to nuke everything and now we're trying to track what we should delete? |
@lynchem is it related to jquery select picker? I can try to reproduce this. |
I really appreciate your thoughtful and thorough review as always Jan. :)
I believe 2.6.1 has fulfilled its purpose solving #374 , #376, #377 and others while producing no major errors/breaking changes. so it's safe to assume we can release this then follow up with another minor release solving remaining issues before 3.0 as you recommended @jankapunkt.
|
This is the library in question Jan |
FYI - I did some performance testing with lighthouse and just by accident found this: meteor/meteor#12085 |
@harryadel I think you are right, we should merge and release and prepare a 2.6.2 release that covers the findings. @lynchem I will create a new issue from your comment. |
@denihs from our end this is done, can you please publish 2.6.1 ? |
It's done! 2.6.1 released. |
blaze@2.6.1 released.
Read more in the milistone.