Comments (9)
The error must be attached I prevented with #366 is raised, when using ostrio:flow-router-extra and move away from a template but the template raises an error in onDestroyed, which just looks like it's related to #213 but in fact it's not. So I think there is still a valid use-case for #366 but the related code may change when implementing a fix for
Huh, that makes sense #366 was not an immediate fix to #213 but basically a solution to a problem you found along the way.
Now ostrio:flow-router-extra also is a bit problematic here, since as stated in #372 it contains some kind of monkey-patching code:
In my opinion we need to use a fork of ostrio:flow-router-extra and re-test all issues to get a side-effect free result here.
I agree with you. Maybe @dr-dimitru can chime in? Though, this insinuates if I removed the monkey patch from ostrio in my test repo, I'm bound to get different results? Let me see.
Some side note, but it's unacceptable to merge in PRs with zero testing as in the case of #366 or #374. A regression test must be added for both.
Finally, to get things moving instead of theorizing further, it may beneficial to merge in #374 (with tests ofc) according to #372 (comment) and release 2.6.1 then come up with 2.6.2 for #213 where we can include additional stuff like #373.
Again, I really appreciate your efforts pushing this through and sorry for dipping on you.
from blaze.
@jankapunkt @harryadel I'm working on update. Closed most of the issues. Can you post here proposed changes or come up with PR?
from blaze.
Thank you for your work @dr-dimitru. I see you already pushed a new version removing the monkey patch. I've reached out to Jan and we're going to look into it and try push things through in the next couple of days.
from blaze.
Hey @jankapunkt, thanks for your work and time.
I'm a bit puzzled with all the back and forth and it helps me to have a specific problem to fix instead of thinking abstractly, so I pulled @distalx reproduction repository, and implemented the two proposed solutions in a locally cloned Blaze package:
https://github.com/meteor/blaze/pull/366/files
if (view._domrange)
view._domrange.destroyMembers(_skipNodes);
Blaze._fireCallbacks(view, 'destroyed');
if (! view.isDestroyed) {
var range = view._domrange;
range.destroy();
if (range.attached && ! range.parentRange)
range.detach();
}
So, would I be mistaken to assume that the current PR doesn't fix the aforementioned problem? Here's the repo I tested with in case you want to give it a spin too. Or the 'Must be attached' problem is completely indifferent to the #372? as @michaelcbrook seems adamant about adding the change.
from blaze.
@harryadel yes the problem in #213 is not solved by #366 and is not directly related to #372, however the current PR #374 is intended only to fix #372 while #213 is not affected by it.
I currently check out the example repo and see what I can find regarding the issue there.
I know that solving for a concrete example sounds reasonable but I'd like to find the root cause of this error so it won't get caused due to a different use-case.
from blaze.
For better understanding:
The error must be attached
in #213 is rather related to jQuery event handling when a sub template is in scope of the parent template. When using ostrio:flow-router-extra
the event is fired when already moved to a new route (thus no DOMRange attached anymore), however it should not fire at all. This is rather a scoping issue to me and also I wonder if we can prevent the event to pass through.
The error must be attached
I prevented with #366 is raised, when using ostrio:flow-router-extra
and move away from a template but the template raises an error in onDestroyed
, which just looks like it's related to #213 but in fact it's not. So I think there is still a valid use-case for #366 but the related code may change when implementing a fix for
Now ostrio:flow-router-extra
also is a bit problematic here, since as stated in #372 it contains some kind of monkey-patching code:
const _BlazeRemove = function (view) {
try {
Blaze.remove(view);
} catch (_e) {
try {
Blaze._destroyView(view);
view._domrange.destroy();
} catch (__e) {
view._domrange.destroy();
}
}
};
And this is also called when the yielded Template is destroyed:
Template.yield.onDestroyed(function () {
if (self.old.template.view) {
_BlazeRemove(self.old.template.view);
self.old.template.view = null;
self.old.materialized = false;
}
self.yield = null;
});
plus on two other lines:
- https://github.com/veliovgroup/flow-router/blob/master/client/renderer.js#L187
- https://github.com/veliovgroup/flow-router/blob/master/client/renderer.js#L205
In my opinion we need to use a fork of ostrio:flow-router-extra
and re-test all issues to get a side-effect free result here.
from blaze.
Though, this insinuates if I removed the monkey patch from ostrio in my test repo, I'm bound to get different results? Let me see.
Not necessarily but I think we need to get rid of side-effects before testing to make sure this is not a bug introduced by the monkey-patch AND we can then also create a solution that may makes the monkey-patch obsolete.
Some side note, but it's unacceptable to merge in PRs with zero testing as in the case of #366 or #374. A regression test must be added for #366 along with #374.
I agree partially. It is not easy for people to setup a local Blaze project and link it to their current projects, which is why I though the RC-releases to be a good solution since they will never automatically get update but require explicit adding via meteor add
or tweaking in .meteor/packages
so only people involved in testing will "touch" the RC
Finally, to get things moving instead of theorizing further, it may beneficial to merge in #374 (with tests ofc) according to #372 (comment) and release #371 then come up with 2.6.2 for #213 where we can include additional stuff like #373.
I also partially agree here. I think we should continue doing RC releases until we got it all tested with some running apps and if it's stable we can make 2.6.1
. There are still many apps out there using Blaze and we should be very sure that they will not break due to a new release.
Again, I really appreciate your efforts pushing this through and sorry for dipping on you.
No offense taken here. I think we need to be very careful in this current situation to also keep the good name of Meteor and Blaze up.
from blaze.
@jankapunkt @harryadel thank you for mentioning me. Sure, I'll run tests on my end and study this thread in detail this week. And update package if necessary to get it compatible with latest Meteor and Blaze releases
from blaze.
This is now covered by release 2.6.1 so I'm closing this.
from blaze.
Related Issues (20)
- spacebars-tests packages still uses removed code
- Blaze.remove() destroys DOM before calling onDestroyed() HOT 8
- Errors in onCreated callback cause a complete stop for rendering further Templates
- Bootstrap select picker is not correctly removed / disposed during Template descruction HOT 6
- Add benchmarks to tests
- SSR is broken in 2.6.0 HOT 4
- Add ts types HOT 3
- Blaze compile errors completely silent if imported HOT 12
- Be able to have more contentBlock in template HOT 1
- Non-primitives not fully reactive in 2.7
- Complete GitHub community standards HOT 7
- CI/CD for documentaiton
- observe-sequence has a bug when _ids have a period HOT 11
- Possibility of a release for people wanting to update to be able to migrate their templates to ASYNC? HOT 4
- Support for async dynamic attributes HOT 3
- Measure performance impact of recent async changes HOT 1
- TypeError: `Cannot convert undefined or null to object` in `waitForAllAttributesAndContinue` HOT 3
- Allowing arbitrary expressions
- Async Helpers are not reactive after the first "await" HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from blaze.