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

Release 2.6.1 #371

Merged
merged 23 commits into from
Jul 22, 2022
Merged

Release 2.6.1 #371

merged 23 commits into from
Jul 22, 2022

Conversation

denihs
Copy link
Contributor

@denihs denihs commented Apr 19, 2022

blaze@2.6.1 released.

Read more in the milistone.

@denihs
Copy link
Contributor Author

denihs commented Apr 19, 2022

blaze@2.6.1-rc.0 is ready to be tested.

@denihs
Copy link
Contributor Author

denihs commented Apr 20, 2022

2.6.1-rc.1 released.

@harryadel
Copy link
Contributor

I think the rc has been out enough, can we release it? I'd like to fix the Travis error in
meteor/meteor#11869

@jankapunkt
Copy link
Collaborator

@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
@StorytellerCZ
Copy link
Collaborator

@jankapunkt anythings else that should be added?

@denihs
Copy link
Contributor Author

denihs commented May 18, 2022

Please let me know when it's ready, and I'll release the official version. 🙌

@lynchem
Copy link

lynchem commented May 23, 2022

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.
Is this the best place to discuss it?


Basically I have a template something like

{{#if someCondition}}
{{> templateWithSelect options=options1}}
{{else}}
{{> templateWithSelect options=options2}}
{{/if}}

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.
The onDestroyed of the template is getting called and in that we're destroying the select .... but if I put any other control / template in there it works as expected so I'm guessing this is an isolated case and purely our problem or a problem with the select lib: https://developer.snapappointments.com/bootstrap-select/

@harryadel
Copy link
Contributor

@lynchem Does this problem still persist? If so, can you please create a reproduction repo so we can look further into it?

@lynchem
Copy link

lynchem commented Jun 14, 2022

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.
I'll open an issue all the same and try get a repro as it might be interesting to understand why it did work and now it doesn't.

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
@jankapunkt
Copy link
Collaborator

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.

@StorytellerCZ
Copy link
Collaborator

@jankapunkt let us know once you are done. @denihs I think we can have one more RC release here.

@denihs
Copy link
Contributor Author

denihs commented Jul 4, 2022

It's done! blaze@2.6.1-rc.2 is out.

@jankapunkt
Copy link
Collaborator

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.

@StorytellerCZ
Copy link
Collaborator

@jankapunkt @denihs if there are no issue then we can release next week.

@jankapunkt
Copy link
Collaborator

@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.

@jankapunkt
Copy link
Collaborator

Okay @denihs @StorytellerCZ @harryadel I did used the rc release in our staging env and this is what I covered:

  • Flow Router works as expected with the latest release 3.8.1 (where the Monkey Patching related to Blaze: Uncaught Error: Must be attached #213 is now removed)
  • We use some packages that deeply hook into some Template and TemplateInstance prototype function; no issues, works as before
  • No DOMRange must be attached errors were found in our projects, however we found an issue with using instance.$(...) where the Template is already rendered but it says that DOMRange is required, however we can't reproduce the error constantly so I think this might have something to do with our own code. I will keep an eye on this

Error handling and search for DOMRange issues

I 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 blaze@2.6.1-rc.2 to .meteor/packages

Then replaced client/main.html with the following:

<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 client/main.js:

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

09:46:04.900 Exception from Tracker recompute function: meteor.js:1064:23
09:46:04.901 Error: Error-1 meteor.js:1064:23
09:46:04.901 module/</<@http://localhost:3000/app/app.js?hash=2589f8fab30c3e63ebc6f027a605b14bdc7a5773:212:11
module/fireCallbacks/<@http://localhost:3000/packages/blaze.js?hash=4e97ea5149f9400d1971a350b259a60cb2bf73a5:3305:20

This caused a follow-up error, which always occurs when Template A is removed:

09:56:33.315 Exception from Tracker recompute function: [meteor.js:1064:23](http://localhost:3000/packages/meteor.js?hash=b9ec8cf25b6fc794ae6b825f30e06c3c35c50e7c)
09:56:33.315 TypeError: domrange is undefined [meteor.js:1064:23](http://localhost:3000/packages/meteor.js?hash=b9ec8cf25b6fc794ae6b825f30e06c3c35c50e7c)
09:56:33.315 doMaterialize@http://localhost:3000/packages/blaze.js?hash=4e97ea5149f9400d1971a350b259a60cb2bf73a5:1952:11

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 - Prod

In production it's worse since nobody knows where this comes from:

Error: Error-1 [144eb911d5f3d4749cb2bd08d2ac42cc9e2c7110.js:1:6618](http://localhost:3000/144eb911d5f3d4749cb2bd08d2ac42cc9e2c7110.js?meteor_js_resource=true)
10:18:03.451 e/<@http://localhost:3000/144eb911d5f3d4749cb2bd08d2ac42cc9e2c7110.js?meteor_js_resource=true:164:2698

And the DOMRange error is completely obfuscated:

10:18:08.468 Exception from Tracker recompute function: [144eb911d5f3d4749cb2bd08d2ac42cc9e2c7110.js:1:6618](http://localhost:3000/144eb911d5f3d4749cb2bd08d2ac42cc9e2c7110.js?meteor_js_resource=true)
10:18:08.468 TypeError: o is undefined [144eb911d5f3d4749cb2bd08d2ac42cc9e2c7110.js:1:6618](http://localhost:3000/144eb911d5f3d4749cb2bd08d2ac42cc9e2c7110.js?meteor_js_resource=true)
10:18:08.469 t@http://localhost:3000/144eb911d5f3d4749cb2bd08d2ac42cc9e2c7110.js?meteor_js_resource=true:144:21604

This is something we definitely need to tackle! I think this will be much better, when we integrate #368

onRendered - dev

9:49:05.012 Exception from Tracker afterFlush function: meteor.js:1064:23
09:49:05.013 Error: Error-2 meteor.js:1064:23
09:49:05.013 module/</<@http://localhost:3000/app/app.js?hash=2589f8fab30c3e63ebc6f027a605b14bdc7a5773:215:11
module/fireCallbacks/<@http://localhost:3000/packages/blaze.js?hash=4e97ea5149f9400d1971a350b259a60cb2bf73a5:3305:20
  • Follow-up Templates can be rendered

onRendered - prod

10:20:05.964 Exception from Tracker afterFlush function: [144eb911d5f3d4749cb2bd08d2ac42cc9e2c7110.js:1:6618](http://localhost:3000/144eb911d5f3d4749cb2bd08d2ac42cc9e2c7110.js?meteor_js_resource=true)
10:20:05.965 Error: Error-2 [144eb911d5f3d4749cb2bd08d2ac42cc9e2c7110.js:1:6618](http://localhost:3000/144eb911d5f3d4749cb2bd08d2ac42cc9e2c7110.js?meteor_js_resource=true)
10:20:05.965 e/<@http://localhost:3000/144eb911d5f3d4749cb2bd08d2ac42cc9e2c7110.js?meteor_js_resource=true:164:2763
  • Follow-up Templates can be rendered
  • This is still somewhat hard to decode, since Tracker afterFlush indicates something in a Tracker or autorun and it's hard to understand that this occurs in onRendered

Helpers - dev

09:53:41.528 Exception in template helper: raise@http://localhost:3000/app/app.js?hash=2589f8fab30c3e63ebc6f027a605b14bdc7a5773:219:13
  • Follow-up Templates can be rendered

Helpers - prod

Exception in template helper: raise@http://localhost:3000
  • This is quite good to understand
  • Follow-up Templates can be rendered

Events - dev

Uncaught Error: Error-4
    click .raise-btn main.js:44
    module/Template.prototype.events/eventMap2[k]/eventMap2[k]< template.js:540
  • Follow-up Templates can be rendered

Events - prod

Uncaught Error: Error-4
    click .raise-btn http://localhost:3000/144eb911d5f3d4749cb2bd08d2ac42cc9e2c7110.js?meteor_js_resource=true:164
    e/c.prototype.events/n[r]/n[r]< http://localhost:3000/144eb911d5f3d4749cb2bd08d2ac42cc9e2c7110.js?meteor_js_resource=true:144
  • This is quite good to understand
  • Follow-up Templates can be rendered

onDestroyed and manual Blaze.remove - dev

  • Select Template E, click "render" then click "remove"
Uncaught Error: Error-5
    module main.js:62
  • Follow-up Templates can be rendered

onDestroyed and manual Blaze.remove - prod

Uncaught Error: Error-5
    e http://localhost:3000/144eb911d5f3d4749cb2bd08d2ac42cc9e2c7110.js?meteor_js_resource=true:164
    f http://localhost:3000/144eb911d5f3d4749cb2bd08d2ac42cc9e2c7110.js?meteor_js_resource=true:144
  • quite hard to get where it comes from unless the error message indicates this
  • Follow-up Templates can be rendered

onDestroyed after automatic remove - dev

  • Select Template F and then another one, like Template D
10:13:57.561 Exception from Tracker recompute function: [meteor.js:1064:23](http://localhost:3000/packages/meteor.js?hash=b9ec8cf25b6fc794ae6b825f30e06c3c35c50e7c)
10:13:57.562 Error: Error-6 [meteor.js:1064:23](http://localhost:3000/packages/meteor.js?hash=b9ec8cf25b6fc794ae6b825f30e06c3c35c50e7c)
10:13:57.562 module/</<@http://localhost:3000/app/app.js?hash=686675d1b8249ef3310c1d66165e334b5273f88e:267:11
  • Follow-up Templates can be rendered

onDestroyed after automatic remove - prod

Exception from Tracker recompute function: [144eb911d5f3d4749cb2bd08d2ac42cc9e2c7110.js:1:6618](http://localhost:3000/144eb911d5f3d4749cb2bd08d2ac42cc9e2c7110.js?meteor_js_resource=true)
10:23:36.014 Error: Error-6 [144eb911d5f3d4749cb2bd08d2ac42cc9e2c7110.js:1:6618](http://localhost:3000/144eb911d5f3d4749cb2bd08d2ac42cc9e2c7110.js?meteor_js_resource=true)
10:23:36.014 e/<@http://localhost:3000/144eb911d5f3d4749cb2bd08d2ac42cc9e2c7110.js?meteor_js_resource=true:164:3183
e/f/<@http://localhost:3000
  • quite hard to get where it comes from unless the error message indicates this, again Tracker recompute does no really indicate an issue in onDestroyed
  • Follow-up Templates can be rendered

My opinion on that

  • 2.6.1 can be released
  • The onCreated error, preventing any further Templates to be rendered is not critical but it's also not something to be ignored; If users don't write messy code then it will not occur; however, if it occurs then it breaks UX since a page refresh is required
  • We should prioritize Improve exception handling #368 for a release 2.6.2 and not 3.0 to improve error handling in production

Copy link
Collaborator

@jankapunkt jankapunkt left a 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
Copy link
Collaborator

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

HISTORY.md Show resolved Hide resolved
@lynchem
Copy link

lynchem commented Jul 13, 2022

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

<template name="multipleSelectFilter">
    <select id="{{queryParamKey}}-select" class="form-control selectpicker show-tick" multiple="multiple"
        data-actions-box="true" data-selected-text-format="count > 3">
        {{#each options}}
            <option value={{value}}>{{label}}</option>
        {{/each}}
    </select>
</template>

In our onRendered we do $(this.jquerySelector).selectpicker(); to initialise the control. So in the DOM instead of just our select I now have.

<div> //new parent added by the control
    <select></select> //our initial select 
    <button></button> //added by the control
</div>

In our onDestroyed we call $(this.jquerySelector).selectpicker("destroy"); but it doesn't clean up any of the new elements it created and leaves the button & enclosing div. I removed the onDestroyed callback entirely incase it was somehow preventing the default destruction from happening and it also only destroys the initial select that was part of our template. So every time I switch views on this page I end up with the old <div><button></button></div> lying around.

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?

@jankapunkt
Copy link
Collaborator

@lynchem is it related to jquery select picker? I can try to reproduce this.

@harryadel
Copy link
Contributor

I really appreciate your thoughtful and thorough review as always Jan. :)

Okay @denihs @StorytellerCZ @harryadel I did used the rc release in our staging env and this is what I covered:
Flow Router works as expected with the latest release 3.8.1 (where the Monkey Patching related to #213 is now removed)
We use some packages that deeply hook into some Template and TemplateInstance prototype function; no issues, works as before
No DOMRange must be attached errors were found in our projects, however we found an issue with using instance.$(...) where the Template is already rendered but it says that DOMRange is required, however we can't reproduce the error constantly so I
think this might have something to do with our own code. I will keep an eye on this

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.

2.6.1 can be released
The onCreated error, preventing any further Templates to be rendered is not critical but it's also not something to be ignored; If users don't write messy code then it will not occur; however, if it occurs then it breaks UX since a page refresh is required
We should prioritize #368 for a release 2.6.2 and not 3.0 to improve error handling in production

@lynchem
Copy link

lynchem commented Jul 13, 2022

@lynchem is it related to jquery select picker? I can try to reproduce this.

This is the library in question Jan

@jankapunkt
Copy link
Collaborator

FYI - I did some performance testing with lighthouse and just by accident found this: meteor/meteor#12085

@jankapunkt
Copy link
Collaborator

@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.

@jankapunkt jankapunkt merged commit ad5c7a8 into master Jul 22, 2022
@jankapunkt jankapunkt deleted the release-2.6.1 branch July 22, 2022 16:01
@jankapunkt
Copy link
Collaborator

@denihs from our end this is done, can you please publish 2.6.1 ?

@jankapunkt jankapunkt linked an issue Jul 22, 2022 that may be closed by this pull request
@denihs
Copy link
Contributor Author

denihs commented Jul 25, 2022

It's done! 2.6.1 released.

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.

Blaze.remove() destroys DOM before calling onDestroyed()
5 participants