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

Fix inline views when template uses unescaped variables #672

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

baerrach
Copy link
Contributor

I'm trying to see if its possible to write a test for the case where the template literal variable is unescaped.

The decorator should get the exception and be able to provide a warning message that the input might have unescaped dollar signs in template literals.

However the current view-strategy.spec.js hacks together a ViewEngine like object. That was simple enough to replace and the tests still work.

You can see from my attempt to create a test for InlineViewStrategy that the .getElement(el) call returns null.

I probably should be testing closer to the decorator usage and missing some magic setup that my lack of familiarity is causing.

Is this something else to debug and fix?

When a test fails under Karma the console stacktrace contains incorrect line
numbers making debugging impossible.

Applying fix manually as described in chriscasola/karma@67d1cfe

closes aurelia#670
@bigopon
Copy link
Member

bigopon commented Jul 13, 2019

@baerrach the changes look good to me. I think if ypu have more ideas, you can add later in another PR. Or you can also keep this open and add more to it.

@baerrach
Copy link
Contributor Author

@bigopon
Are you able to spare a few minutes to work out why the .getElement(el) call fails to find the element then?

@bigopon
Copy link
Member

bigopon commented Jul 13, 2019

@baerrach I missed that. loadViewFactory only loads, that's why you don't have the element registered to the view resources you were testing. Normally all modules are loaded and piped through ModuleAnalyzer, and registered to corresponding ViewResources within that flow.

Though I think the test is still nice, we just need to assert the result of loading like ViewFactory instead

@baerrach
Copy link
Contributor Author

@bigopon I'm blindly transmogrifying the existing tests that use StaticViewStrategy to create my test for InlineViewStrategy and those existing tests only call strategy.loadViewFactory and expect the resources to have been created.

If anyone has some spare time to look at what I am attempting to do, and work out how it should be done then I can take it from there, but I can't make that leap yet as I dont have enough experience with the internal workings of Aurelia.

@baerrach
Copy link
Contributor Author

I've tried using the decorator inlineView to avoid less black magic in the setup.

The View and ViewStrategy appear to be what I expect.

But after .loadViewFactory the factory.resources.elements is still empty.

The test code for view-locator.spec.js only uses StaticViewStrategy.

Where can I find the "ModuleAnalyzer, and registered to corresponding ViewResources" code so I can hook this up better? My end goal is to change

inlineView('<template><input value.bind="value"></template>')(El);

into one that doesn't escape the template literal dollar sign

inlineView(`<template><input value.bind="value">${anUnescapeDollar}</template>`)(El);

And see where the errors get propagated, hopefully catching it in something that can give a decent warning.

@bigopon
Copy link
Member

bigopon commented Jul 15, 2019

For the specific purpose of not escaping the dollar sign, you can escape it yourself?

inlineView(`<template><input value.bind="value">\${anUnescapeDollar}</template>`)(El);
                                                 ^^ escape it here

For module analyzer, it would work similarly like you can see in these tests here https://github.com/aurelia/templating/blob/master/test/module-analyzer.spec.js . For example: in the test at

it('analyses with base using static config and derived using metadata', () => {

we are simulating a module returned by the loader, and pass it through the module analyzer. Then those resource analysis will be registered with a ViewResources instance. The actual registration step will be done later, but what you want to achieve does not seem to need it.

@baerrach
Copy link
Contributor Author

baerrach commented Jul 15, 2019

My goal is to see if I can write a test for the unescaped case and see where the error propagates.

While doing some of the examples I found myself using an inline view because that is what the example used. But I needed to modify it to refactor the repeat block and something else and the refactoring was from the view file.

Copy-and-paste between a view and an inline template isn't 1-to-1, and its an easy mistake to make.

That's when I found #252.

I know this is an edge case, and one that hardly anyone is going to bump into, but it is giving my some exposure to the internals of Aurelia and how it does testing. So I thought it would be worthwhile seeing if its was possible to get Aurelia to warn when someone does this, but to do that I need to work out where the template literal fails on an unbound variable occurs and whether that is within inlineView or in the outer scope. I'm a bit hazy on JavaScript scoping rules here.

Are you sure I want a module analyzers? This just looks like it peeks inside a module to work out which one is the main resource and which ones are additional resources.

Don't I want to do something like https://github.com/aurelia/templating/blob/master/test/view-strategy.spec.js#L193 where I load a view and check that it is bound properly?

I'm also fairly sure that these test throw exceptions and don't actually run the expect checks.
It looks like it is succeeding because https://github.com/aurelia/templating/blob/master/test/view-strategy.spec.js#L193 has expect(ex.message).not.toContain which is true but not what you want to be testing.

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.

None yet

2 participants