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

Observable change error, when loading jquery.observable.js before jsrender.js, and using contextual parameters and sorted {{for}} #446

Closed
johan-ohrn opened this issue May 17, 2020 · 6 comments

Comments

@johan-ohrn
Copy link

johan-ohrn commented May 17, 2020

Sorry about the poor title.
Anyway I found this issue.

I have this for-tag:

{^{for ~blah="apa" ~tag.tagCtx.filteredCollection.filtered start=~tag.tagCtx.pagingStartIdx end=~tag.tagCtx.pagingEndIdx tmpl=~tag.rowTemplate /}}

I do something to trigger pagingEndIdx to change and this ends up calling this code in jquery.observable.js:

			observe: function(deps, linkCtx) { // Listen to observable changes of mapProps, and call map.update when change happens
				var map = this,
					options = map.options;
				if (map.obmp) {
					// There is a previous handler observing the mapProps
					$unobserve(map.obmp);
				}
				map.obmp = function() {
					// Observe changes in the mapProps ("filter", "sort", "reverse", "start", "end")
					var newTagCtx = linkCtx.fn(linkCtx.data, linkCtx.view, $sub)[options.index]; // Updated tagCtx props and args
					$.extend(options.props, newTagCtx.props); // Update props to new values
					options.args = newTagCtx.args; // Update args to new values
					map.update(); // Update the map target array, based on new mapProp values
				};
				$observable._apply(1, linkCtx.data, dependsPaths(deps, linkCtx.tag, map.obmp), map.obmp, linkCtx._ctxCb);
			},

The line var newTagCtx = linkCtx.fn(linkCtx.data, linkCtx.view, $sub)[options.index]; // Updated tagCtx props and args is the culprit.

$sub does not contain everything the compiled view function expect. If I instead change the above line to this: var newTagCtx = linkCtx.fn(linkCtx.data, linkCtx.view, $.views.sub)[options.index]; // Updated tagCtx props and args then it work.

The compiled view function look like this:

(function anonymous(data,view,j,u
) {
 // #Tags.DataGrid/if 1 for
return [
{view:view,content:false,tmpl:false,
	params:{args:['~tag.tagCtx.filteredCollection.filtered'],
	props:{'start':'~tag.tagCtx.pagingStartIdx','end':'~tag.tagCtx.pagingEndIdx','tmpl':'~tag.rowTemplate'},
	ctx:{'blah':'\"apa\"'}},
	args:[view.ctxPrm("tag").tagCtx.filteredCollection.filtered],
	props:{'start':view.ctxPrm("tag").tagCtx.pagingStartIdx,'end':view.ctxPrm("tag").tagCtx.pagingEndIdx,'tmpl':view.ctxPrm("tag").rowTemplate},
	ctx:{'blah':j._cp("apa","\"apa\"",view)}}]
})

Notice how it wants to call j._cp(...). That function is not present on $sub but it is on $.views.sub

I hope you can spot the error and a fix for it. If not I can try to reproduce the error with simpler code.

@BorisMoore
Copy link
Owner

BorisMoore commented May 18, 2020

Thanks, Johann. Another good find. That is a bug which arises in the case of having jquery.observable.js and jsrender.js and jquery,views.js as separate files, with jquery.observable.js loaded before jsrender.js. I have repro.

I believe that if you load jsrender.js first or use jsviews.js (single file) the issue will not occur.

I'll work to get a fix, then let you know here...

@BorisMoore BorisMoore added the Bug label May 18, 2020
@BorisMoore BorisMoore changed the title observable change error observable change error, when loading jquery.observable.js before jsrender.js May 18, 2020
@BorisMoore BorisMoore changed the title observable change error, when loading jquery.observable.js before jsrender.js Observable change error, when loading jquery.observable.js before jsrender.js, and using contextual parameters and sorted {{for}} May 18, 2020
@johan-ohrn
Copy link
Author

Great! Once I have this fixed I can push the new version into production.

@BorisMoore
Copy link
Owner

Hi Johan, I think this should fix the issue:

jsviews2.zip

Let me know if it works for you.

(It also includes a fix to jsrender-node.js, and your issue #447 (comment))

@johan-ohrn
Copy link
Author

An other related potential? issue is the following. In jquery.views.js on line 3184:

contextOb = contextOb && $sub.tmplFn(delimOpenChar1 + ":" + contextOb[1] + delimCloseChar0, view.tmpl, true)(linkCtx.data, view);

The compiled function returned by tmplFn() have both j and u input parameters but the call only passes in data and the view. There are a number of places where a similar call is made without passing in the extra parameters. I can't actually remember if I had an issue with it or not.
Just letting you know for your consideration.

@BorisMoore
Copy link
Owner

You are right!! Thanks...

It is difficult to make a bug scenario that encounters that as an issue, but I managed to do so with this:

{^{on ~util[0].swap ...}}

which I will add to the unit tests.

So I have fixed it in the update below, to pass in $sub:

contextOb = contextOb && $sub.tmplFn(delimOpenChar1 + ":" + contextOb[1] + delimCloseChar0, view.tmpl, true)(linkCtx.data, view, $sub);

The u parameter is intended to be undefined, but in this update I have eliminated the ,u from the generated functions. I previously intended to use u for undefined in generated code, but I don't seem to be leveraging that any more, so I think it is OK to remove it.

Here is the update:

jsviews3.zip

BorisMoore added a commit to BorisMoore/jsviews.com that referenced this issue Jul 12, 2020
Feature improvements for JsRender on Node.js:

  (See issue BorisMoore/jsrender#357)

- File based templates for JsRender on Node.js now allow
  absolute paths

  See the updated documentation topic:
  https://www.jsviews.com/#node/filetmpls@htmlfile

- The renderFile() method for JsRender on Node.js
  now allows passing of context and helpers as well
  as data

  See the new documentation topic:
  https://www.jsviews.com/#node/filetmpls@renderfilehlelpers

Performance optimization changes:

- BorisMoore/jsviews#448
  $.observe/$.unobserve performance degradation over time

- BorisMoore/jsviews#447
  removeView performance

Several bug fixes, including:

- BorisMoore/jsviews#446
  Observable change error, when loading jquery.observable.js before
  jsrender.js, and using contextual parameters and sorted {{for}}

- BorisMoore/jsviews#440
  linked tag property changed handler called when it shouldn't

- BorisMoore/jsviews#445
  jsrender.min.js.map truncated

Additional small corrections or improvements to documentation
and some additional unit tests...
BorisMoore added a commit that referenced this issue Jul 12, 2020
Feature improvements for JsRender on Node.js:

  (See issue BorisMoore/jsrender#357)

- File based templates for JsRender on Node.js now allow
  absolute paths

  See the updated documentation topic:
  https://www.jsviews.com/#node/filetmpls@htmlfile

- The renderFile() method for JsRender on Node.js
  now allows passing of context and helpers as well
  as data

  See the new documentation topic:
  https://www.jsviews.com/#node/filetmpls@renderfilehlelpers

Performance optimization changes:

- #448
  $.observe/$.unobserve performance degradation over time

- #447
  removeView performance

Several bug fixes, including:

- #446
  Observable change error, when loading jquery.observable.js before
  jsrender.js, and using contextual parameters and sorted {{for}}

- #440
  linked tag property changed handler called when it shouldn't

- #445
  jsrender.min.js.map truncated

Additional small corrections or improvements to documentation
and some additional unit tests...
@BorisMoore
Copy link
Owner

This has been fixed in new release v1.0.7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants