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

can.view Breaking backward compatibility in v2.0.5 amd #717

Closed
steeleprice opened this issue Feb 5, 2014 · 21 comments
Closed

can.view Breaking backward compatibility in v2.0.5 amd #717

steeleprice opened this issue Feb 5, 2014 · 21 comments
Milestone

Comments

@steeleprice
Copy link

I am having a major issue with v2.0.5.

I have a working App that has had no other change except a straight replacement of v2.0.4 with v2.0.5 AMD.

These statements used to work:

$(element).html(can.view(require.toUrl("comp/home/home.html"), {}));
$('body').append(can.view.mustache("<app-login headers='session.securityHeader' token='session.token' notifications='session.notifications' rememberme='session.persistToken'></app-login>")({ session: attr.session }));

Now they both fail in scanner.js at line 249 with Object doesn't support property or method 'replace'

This is because source is a document fragment.
I can get around the first by first saving the view to a variable then setting html to the frag, but this seems weird to have to do when I never had to previously.
The second sample is even used in all the documentation for Components.
Am I missing something that documents this and need to change my syntax?
I'd consider this a pretty big deal since I would have to change at least dozens of places views are loaded.

Thanks

@justinbmeyer
Copy link
Contributor

Does it work if your template ends with .mustache ?

Sent from my iPhone

On Feb 4, 2014, at 11:24 PM, Steele Price notifications@github.com wrote:

I am having a major issue with v2.0.5.

I have a working App that has had no other change except a straight replacement of v2.0.4 with v2.0.5 AMD.

These statements used to work:
$(element).html(can.view(require.toUrl("comp/home/home.html"), {}));
$('body').append(can.view.mustache("")({ session: attr.session }));

Now they both fail in scanner.js at line 249 with Object doesn't support property or method 'replace'

This is because source is a document fragment.
I can get around the first by first saving the view to a variable then setting html to the frag, but this seems weird to have to do when I never had to previously.
The second sample is even used in all the documentation for Components.
Am I missing something that documents this and need to change my syntax?
I'd consider this a pretty big deal since I would have to change at least dozens of places views are loaded.

Thanks


Reply to this email directly or view it on GitHub.

@steeleprice
Copy link
Author

Let me try that, but it seems to only be an issue in AMD.
I created a fiddle using latest/can and it works as expected.
I really need to create a fiddle template soon for AMD, but I can't do it today.

@daffl
Copy link
Contributor

daffl commented Feb 5, 2014

What happens if you just use

$(element).html(can.view("comp/home/home.html", {}));

This could have been introduced because of #647

@steeleprice
Copy link
Author

.mustache didn't work, it's still a fragment instead of a string as it is in 2.0.4.

daffl: that works, but is really strange, require.toUrl just changes the path to "scripts/app/components/home/home.html"
if I pass anything but a string literal, it's failing, i.e.

                var template = require.toUrl("comp/home/home.mustache");
                $(element).html(can.view(template, {}));

which shows template is a string variable containing: "scripts/app/components/home/home.mustache"
It seems related to #647 but isn't a require base path problem, the problem is that anything but view("string literal", {}) is not rendering.

This fails too:

                var template = "scripts/app/components/home/home.html"
                $(element).html(can.view(template, {}));

@steeleprice
Copy link
Author

in line 652 of view.js, using the above syntax,
text is undefined and id is a fragment.

@justinbmeyer
Copy link
Contributor

I built an example that works.

<html>
    <body>
        <div id='element'></div>
        <script src="require.js"></script>
        <script>
            require(['can','jquery'], function(can, $){
                $('#element').html(can.view("home.html", {}));


                $('body').append(can.view.mustache("Hello")({ session: {} }));
            });
        </script>
    </body>
</html>

@justinbmeyer
Copy link
Contributor

Changing it to:

var str = "home.html"
$('#element').html(can.view(str, {}));

works too

@steeleprice
Copy link
Author

wait a minute...
I'm inside a Component, but I think I see what is going on.
There is an async require for google maps in a sub component that is timing out.

@steeleprice
Copy link
Author

no, even removing that isn't helping, but I do think this is Component related, most of the Components are loading when stepping through.

@justinbmeyer
Copy link
Contributor

@steeleprice ping me on skype / ghangout if you get your code to a minimal breaking case. I was just about to update the site to show 2.0.5. But I want to hold off if it's breaking.

@steeleprice
Copy link
Author

I'm working on a fiddle to emulate it.
I have something close, but it's working, I will adjust it in a few minutes.

@steeleprice
Copy link
Author

ok, so it appears that views don't work quite the same as they used to, but only inside of a component. I am OK with this since all it required was directly calling mustache.
this worked in 2.0.4 inside of a component:

$(element).html(can.view(require.toUrl("comp/home/home.html"), {}));

it doesn't in 2.0.5.
but this does and is better syntax anyway:

$(element).html(can.view.mustache(require.toUrl("comp/home/home.html"))({}));

I still have this error being thrown with the following line though and I am trying to track down exactly what is causing it, this line did in fact render correctly in 2.0.4.

$("body").append(can.view.mustache("<app-location></app-location>")({ session: attrs.session }));

This seems to be a tightening of what is accepted for rendering which is mostly a good thing.
debugging the root cause is quite painful.

@justinbmeyer
Copy link
Contributor

@steeleprice I'm not sure you are correct. The following works for me:

<html>
    <body>
        <div id='element'></div>
        <script src="require.js"></script>
        <script>
            require(['can','jquery'], function(can, $){

                var str = "home.html"
                $('#element').html(can.view(require.toUrl("home.html"), {}));


                $('body').append(can.view.mustache("Hello")({ session: {} }));
            });
        </script>
    </body>
</html>

Please reach out to me so I can stop worrying about this and make 2.0.5 official.

@justinbmeyer
Copy link
Contributor

Wait ... why would you do:

$(element).html(can.view(require.toUrl("comp/home/home.html"), {}));

inside a component? This seems like a mistake. You should not be rendering things inside a component.

@justinbmeyer
Copy link
Contributor

Btw, I am writing an article about how we are going to be backwards compatible / use semantic versioning: https://gist.github.com/justinbmeyer/8812069

So if there is a breaking issue, I want to fix it ASAP so I'm not undermining the entire article :-)

@steeleprice
Copy link
Author

this is definitely a combination problem of Require and setting Component templates.
the rendering inside the component is not the issue, but I will address that too.
The problem was how template: was being called.
ALL my components used:
template: can.view(require.toUrl("comp/home/home.html")),
which in 2.0.5 is
A) no longer needed since it can use a require path directly
and
B) a breaking change, because if you used it in a previous version it's now trying to resolve the path twice.
correct syntax for 2.0.5 is:
template: can.view("comp/home/home.html"),

as for rendering in the component, normally, yes, this is not done, <app-components> are inside the template. In fact, right now <app> doesn't have a template and I am most likely going to change that shortly. I will have to play with that approach. This is all a conversion from a non-component architecture where everything was controls before.
If you have a single root <app> component, it may need to so some rendering for initial setup that is potentially outside the <app> tag

I have to put login, logout and location finders in the body so they don't get clobbered when the hash changes and replaces content inside the <app> tag.

There is really no perfect way to do something like this, it's going to be a trade-off somewhere.
if I created an index page something like this:

<app />
<app-login />
<app-logout />
<app-location />

then I have to reference app.scope.session from inside the other 3 components which now tightly couples them to an app component.
if they are instantiated inside of <app>, they need to be placed in the DOM outside of the tag since routing will replace things inside the tag.

If I want <app> as a root component (as opposed to a control), I am not seeing a good way, except maybe using

<div id="nav">
<app-nav-bar/>
</div>
<div id="content" />
<div id="otherstuff">

inside the template.

This is where Documentation of sample architectures for real-world apps is going to be enormously beneficial.

We are guessing at best-practices right now using "Todos" as a template when we need to think of things outside that, like security, session observes, communication pipelines between components without a facade or sandbox (pub/sub), etc.

@justinbmeyer
Copy link
Contributor

Ok, I tracked this down to this pull request:

#647

As supporting baseUrl was never documented, this is a new feature and should have been part of 2.1 or 3.0.

@stevenvachon and @daffl I think this is still not documented, anyway we can add this to can.view's docs?

@justinbmeyer justinbmeyer added this to the 2.1.0 milestone Feb 5, 2014
@justinbmeyer
Copy link
Contributor

@steeleprice

Don't you mean correct syntax for 2.0.5 is:

template: can.view(require.toUrl("comp/home/home.html")),

Sorry about the break. We are trying very hard to prevent that.

@steeleprice
Copy link
Author

No, this is a GOOD change and just needs to have some documentation to close this issue.
correct syntax is:
template: can.view("comp/home/home.html"),
comp is a requireJS path, not just the base.
in my require setup it look like this:

require.config({
    baseUrl: 'scripts',
    paths: {
        app: 'app',
        controllers: 'app/controllers',
        comp: 'app/components',
        can: 'lib/can',
...

@daffl daffl modified the milestones: 2.0.6, 2.1.0 Feb 10, 2014
@daffl
Copy link
Contributor

daffl commented Feb 25, 2014

So #718 adds documentation for this. If anybody thinks it is inadequate please reopen this issue.

@daffl daffl closed this as completed Feb 25, 2014
@steeleprice
Copy link
Author

There is only one other odd thing I noticed in this issue, but its not really the same issue and would also be solved with documentation.

self closing tags have unexpected behavior in most browsers.
they nest as children of the last element instead if siblings as they should.
Its also inconsistent and I will need to open a new issue for it.

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

3 participants