An anchor tag in dialog-body causes infinite loop

[larserikfinholt]

I'm submitting a bug report

• Library Version:
  1.0.0-rc.1.0.1

Please tell us about your environment:
Windows 10

• Node Version:
  6.10.0

• NPM Version:
  4.0.5

• Webpack
  webpack 2.4.1

• Browser:
  chrome

• Language:
  TypeScript 2.3

Current behavior:
Dialog goes into recurisve loop and terminates with a stackoverflow exception when there is an anchor tag in the dialog body

Expected/desired behavior:
Using this view:

    <ux-dialog>
        <ux-dialog-header>
            <h2>
                Send forespørsel om time
            </h2>
        </ux-dialog-header>
        <ux-dialog-body>

            <button click.trigger="modifyDate()">Works fine</button>
            
            <!-- removing this line, and everything works fine -->
            <a href="#">Trubble</a>
            

        </ux-dialog-body>
        <ux-dialog-footer>
            <button click.trigger="controller.cancel()">Cancel</button>
            <button click.trigger="controller.ok()">Ok</button>
        </ux-dialog-footer>
    </ux-dialog>


// my model
import { DialogController } from 'aurelia-dialog';
@autoinject
export class RequestDialogNew {
    constructor(private controller: DialogController, private stateService: StateService) {
        // console.log("a", this.controller, this.stateService); // will be printed a few hundred times (stack trace not usable)
    }
}

// I'm lanching the dialog using this code
import { RequestDialogNew } from './../request-dialog/request-dialog';
// ...
 this.dialogService.open({
            viewModel: RequestDialogNew,
            model: toShow,
            lock: false
        });```

[StrahilKazlachev]

Will check after #281, probably the same cause.

[StrahilKazlachev]

I'm not able to reproduce this - Chrome 58.0.3029.81. Does it crash when you open it or after some interaction? A repro is appreciated.

[larserikfinholt]

Edit: also created this: https://github.com/larserikfinholt/aurelia-dialog-err

ok, did take the latest typescript webpack skeleton, and reproduced with the following (I'll add each step, maybe you can spot where I'm going wrong)

index.ejs
Added <div aurelia-app="main"> (instead of having on body)

package.json
Added "aurelia-dialog": "^1.0.0-rc.1.0.2",

main.ts
Added .plugin(PLATFORM.moduleName('aurelia-dialog'), config => { config.useDefaults(); config.settings.lock = true; config.settings.centerHorizontalOnly = false; config.settings.startingZIndex = 5; config.settings.keyboard = true; });

welcome.html
Added <button type="button" click.trigger="open()">open</button> (after submit button)

welcome.ts
Modified:

    import { RequestDialogNew } from './request-dialog';
    import { autoinject } from "aurelia-framework";
    import { DialogService } from "aurelia-dialog/dist/commonjs/dialog-service";

    @autoinject
    export class Welcome {
      heading: string = 'Welcome to the Aurelia Navigation App';
      firstName: string = 'John';
      lastName: string = 'Doe';
      previousValue: string = this.fullName;

      constructor(private dialogService: DialogService) { }
      open() {
             this.dialogService.open({
                viewModel: RequestDialogNew,
                model: {jalla:123},
                lock: false
            });

      }

request-dialog.html

         <ux-dialog>
            <ux-dialog-header>
                <h2>Send forespørsel om time</h2>
            </ux-dialog-header>
            <ux-dialog-body>
                   <button click.trigger="modifyDate()">Works fine</button>
                <!-- removing this line, and everything works fine -->
                <a href="#">Trubble</a>
            </ux-dialog-body>
            <ux-dialog-footer>
                <button click.trigger="controller.cancel()">Cancel</button>
                <button click.trigger="controller.ok()">Ok</button>
            </ux-dialog-footer>
        </ux-dialog>` (inside a template tag)

request-dialog.ts

    import { DialogController } from 'aurelia-dialog';
    import { autoinject } from "aurelia-framework";
    @autoinject
    export class RequestDialogNew {
        constructor(private controller: DialogController) {
             console.log("a", this.controller); // will be printed a few hundred times (stack trace not usable)
        }
    }

Error shows up when clicking the button.

[StrahilKazlachev]

@jods4 @EisenbergEffect Some help would be appreciated. I can not reproduce with non-webpack build. The recursion starts in aurelia-templating return m.create then it loops through:

1. HtmlBehaviorResource.prototype.create
2. ViewFactory.prototype.create
3. view-factory export function applyInstructions

Loops back to 1.
Another thing I see is that the <a> element gets assigned an au-target-id attribute and later on selected by fragment.querySelectorAll('.au-target'), this does not happen in the non-webpack build.

[jods4]

I don't see how that could be even remotely linked to Webpack...
Surely Webpack is not the only difference between a working build and a broken one, is it?

What strikes me as odd is that this <a> gets tagged with .au-target.
I'm no expert in our view compiler, but doesn't that mean that Aurelia thinks this tag has a behavior applied -- i.e. that it is a custom element or that it has a custom attribute?

Which HtmlBehaviorResource is created in the loop at step 1?
And which element in the template triggers that creation at step 3?

[StrahilKazlachev]

@jods4 here is simplified repro and a cli setup using the same sources.

[StrahilKazlachev]

@jods4 in the bundle in CompositionEngine.prototype.ensureViewModel function body I see this
var m = __WEBPACK_IMPORTED_MODULE_1_aurelia_metadata__["a" /* metadata */].getOrCreateOwn(__WEBPACK_IMPORTED_MODULE_1_aurelia_metadata__["a" /* metadata */].resource, HtmlBehaviorResource, context.viewModel.constructor);
I don't know what __WEBPACK_IMPORTED_MODULE_1_aurelia_metadata__["a" /* metadata */] is, can you explain?

[jods4]

It's the compiled import/export. Everything is renamed, Webpack leaves comments so that you can figure out what is what.

__WEBPACK_IMPORTED_MODULE_1_aurelia_metadata__ is aurelia-metadata.
"a" is its metadata export.

So I read the code above like:

import { metadata } from "aurelia-metadata";
var m = metadata.getOrCreateOwn(metadata.resource, HtmlBehaviorResource, context.viewModel.constructor);

[StrahilKazlachev]

@jods4 I found the issue, the HtmlBehaviorResource created for the dialog vm class RequestDialogNew gets an elementName of a in the webpack case. In the cli case it is request-dialog-new.
ModuleAnalyzer.prototype.analyze uses the fallbackKey when creating the resource. And webpack has rewritten the export names, the cli hasn't.

[jods4]

@StrahilKazlachev OK I see.

Some background:

• Webpack does rename exports for minimal size.
• Aurelia has a dependency on exported names if you use conventions, e.g. MyAwesomeButtonCustomElement is interpreted as a custom element <my-awesome-button>.
• Dependencies created with the Aurelia plugin (through PLATFORM.moduleName, <require>, ModuleDependenciesPlugin and co.) preserve the export names for that reason.

But the tricky part of aurelia-dialog is that it tends to refer to ViewModels directly rather through the plugin.

Typical pattern for a normal custom element is to register it through
.globalResources(PLATFORM.moduleName('controls/awesome-button'))
or using it locally with
<require from="controls/awesome-button">.

In contrast, typical pattern with aurelia-dialog is to do:

import { MyDialog } from 'dialogs/whatever';
dialogService.open({ viewModel: MyDialog });

And this is pure ES imports, pure Webpack. The name of MyDialog won't be preserved.

I've come to hate Aurelia's reliance on class and module names, they really don't play well with modern bundling tools. I wish we could just deprecate those approaches.
In the meantime, @customElement, @customAttribute can be used to avoid implicit / convention stuff.

I am not familiar with aurelia-dialog, what do you do internally with the ViewModel? Why is it converted into a HTML Behavior and can't you give it specific name or avoid turning it into a custom element? Something doesn't feel good here.

A stopgap measure is to register your dialog ViewModels as Aurelia dependencies. For example by placing a PLATFORM.moduleName("dialogs/whatever") anywhere (preferably in the same file as the import).

[StrahilKazlachev]

@jods4 Mmm, we basically delegate all the lifting to the CompositionEngine.

1. If viewModel is
   • constructor function - use Origin to acquire the module name and fall to the case bellow.
   • string - it is the module name, do nothing
     • call CompostionEngine.prototype.ensureViewModel with the module name, it will return the instantiated VM
   • is object - do nothing - the view model is already instantiated
2. call compositionEngine.compose() - the VM is already instantiated.

[StrahilKazlachev]

@larserikfinholt Not a bug in the dialog.
I tried what @jods4 suggested and it works, use PLATFORM.moduleName('./request-dialog'); where you do import { RequestDialogNew } from './request-dialog';, not nice, but a workaround.
When we have time we will check if there is something we can rework in the dialog itself to avoid this scenario from happening.

[larserikfinholt]

@StrahilKazlachev Thanks for you attention to this issue, and for finding a workaround.
You might add a note about this in the dialog docs.

Thanks again for your contributions to aurelia.

[jods4]

@StrahilKazlachev Maybe we should open an issue in aurelia-templating?

I'm no expert, but my understanding of this code:
https://github.com/aurelia/templating/blob/7693dbd65e59e428e4052922920145b76240f3cf/src/module-analyzer.js#L253
is that the module analyzer will consider that if your export has no decoration, no metadata, does not follow any convetion... then it's a custom element given by its hypenated export name.
I think that's how we ended up with <a> custom element.

I can't see a good use for that fallback and it's obviously a dangerous behavior.
Maybe we should change it.
⚠️ That would break backward compatibility I guess? What are the legitimate use cases?

[EisenbergEffect]

Aurelia does not rely on class names. It relies on export names which should be preserved since they are part of the public API contract for a module.

[jods4]

Right. It never looks at the class name directly but at the module export name.
Few users will notice the difference because everyone does export class X.
In fact, from our own documentation on custom elements, notice the language used (emphasis mine):

Aurelia will take the JavaScript class name, strip CustomElement from the end,

Depending on module names and export names is a pain point of our Webpack setup. It creates problems and prevents optimizations. But we have to live with it for the time being.

It relies on export names which should be preserved since they are part of the public API contract for a module.

A library has a public API surface. A bundled application not so much.
ES import is purposefully designed to enable those optimizations such as tree shaking (removing unused exports) and renaming exports.
Rollup does the same. In fact it goes further as it removes the modules altogether (scope hoisting). This feature is on the roadmap for Webpack as well.

@larserikfinholt Here's another workaround that should work: use import * as x from 'vm' and then x.RequestDialogNew should work, because when a module is used as an object Webpack doesn't rename or remove its exports.

I see two fundamental issues at play here:

1. aurelia-loader is an opaque abstraction that prevents Webpack from understanding what is happening at runtime. moduleName and aurelia-webpack-plugin are our solution to bridge this gap.
   Those tools tell Webpack about the dependencies, but they also tell Webpack that we use modules dynamically and that it should not remove or rename anything.
   The pitfall with aurelia-dialog is that the pattern above is very natural but doesn't use those facilities. Webpack sees it as normal code and it optimizes accordingly.
2. aurelia-templating creates a local custom element based on the export name. For a regular component this doesn't seem right, it's just bugs waiting to happen!. What would have happened if I named my export "a" on purpose? Same bug. Why shouldn't I do that? I see no reason not to.

@EisenbergEffect I think 2 is a bug in aurelia-tempalting. Changing this behavior will also fix this bug right here. Except for the unwanted <a> custom element that messes up the template, everything does work. The proof is that if you don't put <a> in the template there is no problem.

[larserikfinholt]

In case others have the same trouble. A few workarounds has been presented in this thread, I ended up with this one that works fine:

In request-dialog.ts

 constructor(private controller: DialogController) {
    PLATFORM.moduleName('./request-dialog');
 }

[jods4]

That's a work-around but I think it's less than ideal and I'm pretty sure others will hit this.

@larserikfinholt
Now that it works, could you please try to add an unknown element
<request-dialog-new>test</request-dialog-new>
in your template (where the <a> is).
I suspect it will break and that would be proof that there is a bug in aurelia-templating.

[StrahilKazlachev]

Tracked in aurelia/templating#549

[eamodio]

We've hit the same issue moving from our previous systemjs/jspm setup to webpack. Is there a way to get webpack to stop shortening the names for these modules?

[StrahilKazlachev]

@eamodio Did you try using PLATFORM.moduleName() in any of the suggested ways, for example?

[eamodio]

@StrahilKazlachev thanks for the quick reply! Yeah, we tried the PLATFORM.moduleName() and it does indeed work, but I was more asking if there was a more global/configuration setting to get the same behavior without having to explicitly tag each case.

[StrahilKazlachev]

As far as I've understood - no. @jods4 ☝️ am I wrong?

[jods4]

@eamodio I don't think so but I would need to dive into Webpack source code to be sure.

You can try to work-around it in a few ways, though...

• Modules that are declared as Aurelia dependencies somewhere (with moduleName or ModuleDependenciesPlugin for example) should not have their exports renamed.
• Same for modules that are import * as X from "..."
• Only ES export are renamed. So if for example you compile with TS then targeting CommonJS or AMD would avoid the issue.

This is far from ideal and the bug is pretty bad. I don't think it is receiving the attention it needs, you can make yourself heard in aurelia/templating#549.

[eamodio]

@jods4 Thank you for the follow up. We weren't able to get import * as X from "..." to work -- seem to have the same error, but moduleName worked, and might check out ModuleDependenciesPlugin -- thanks for that info.

Yeah, agree that the bug is pretty bad -- we lost quite a bit of time going nuts before we found this issue. Though I'm not sure I'm totally understanding the issue aurelia/templating#549 or more accurately the implications of a change. If that change were to be made, then any class that was to be used as a custom element in a template would need to be named *CustomElement or have the @customElement decorator? If so, that would certainly break our whole project, because we don't have either of those -- and my guess is a lot of other aurelia users wouldn't either, since they aren't required (I honestly didn't even know about either of them)

Is another option here, a blacklist of known/common html elements to avoid this issue? Or some other way to deal with it?

[jods4]

That seems pretty bad.

So any exported class in your resources is a custom element? Why not a custom attribute, value converter or binding behavior?

The breakage will bad indeed, where did you get the idea to go like this?
I think the docs have always been explicit that you should either follow a (customizable) naming convention or use decorators?

[eamodio]

We do use the conventions for the value converters and the decorators for attributes, but we've never used them on elements -- mainly because we didn't know about it (and it works without them), but also thinking about it now, that would mean most of the classes in our app would have CustomElement tacked on the end.

I swear I've seen demos by Rob that never used the CustomElement suffix nor decorators. Here is an old one: https://channel9.msdn.com/Events/Seth-on-the-Road/DevIntersection-2015/Rob-Eisenberg-on-Aurelia at around ~10:45 he has a Hello class and later on he uses it in a template <hello></hello>

Also from the docs on the Aurelia site: http://aurelia.io/hub.html#/doc/article/aurelia/templating/latest/templating-custom-elements/3

This means that, if you wish, you may ignore the Aurelia naming convention for your custom elements. In the example above, we could have simply named the class SecretMessage. The custom element would still be named secret-message.

Granted it follows up with

Given this capability, it might be considered wise to utilize Aurelia's naming convention for custom elements or use the customElement decorator to be explicit when creating a component that is only meant to be used as a custom element and not as a standalone page.

[jods4]

@eamodio good points, I will add a comment in the other issue, because it's where this problem is currently tracked.

[eamodio]

Since it seems that aurelia/templating#549 would be quite a breaking change, since it is a "supported" (even if slightly discouraged) scenario -- is there a much easier (low risk) fix here?
Without breaking things it doesn't seem like you can reasonably stop people from exporting a module that matches a dom thing -- but honestly if you do that -- you are on your own. But because in this case it isn't the user naming it, it is webpack changing the names -- could the template stuff just be protected from using single-letter names? Which I would think would cover the majority (if not all) of these issues caused by webpack name shortening. OR is there way to to control the webpack name shortening to include some prefix (I have no idea of the allowable characters in a module name -- but say $) so that they would never conflict with a dom element?

[eamodio]

@jods4 doh -- should I move my last comment over to that issue too?

[Xceno]

Hey folks, just to let you know:
I've just moved a project over to webpack, based on the latest skeleton and ran into this bug. The workarounds described above work, but I still run into the stackoverflow exception described above, as long as I have any <a>-tag in the modal.

So I'll comment them out for the moment, but is there still no workaround / solution for this?

[jods4]

The cause of this problem is identified but not fixed... It's currently tracked by aurelia/templating#549.

The workarounds above should work, though.
The culprit is the following pattern:

// DON'T
import { MyDialog } from 'dialogs/whatever';
dialogService.open({ viewModel: MyDialog });

// DO
dialogService.open({ viewModel: PLATFORM.moduleName('dialogs/whatever') });

// ...or at least combine them into:
import { MyDialog } from 'dialogs/whatever';
PLATFORM.moduleName('dialogs/whatever');
dialogService.open({ viewModel: MyDialog });

Independently of this bug, more problems have arisen when using the first pattern with Webpack.
For example it's not compatible with the new Webpack 3 ModuleConcatenationPlugin and it might also break with webpack DllPlugin.

Bottom line: when using Webpack, always use moduleName on your dialog viewmodels, avoid importing them with ES import.

[Xceno]

Thanks again for summing it all up @jods4!

Though I think I phrased my question wrong. The workarounds for loading viewmodels work well.
Instead I was wondering if there is a workaround for using <a>-tags in the viewModel. Currently i use buttons and some JS to open links.

[jods4]

You should be able to use <a> tags if you load the VM with moduleName... doesn't it work?

[Xceno]

No, I still run into an infinite loop. Here's what I do:

package.json

"dependencies: {
  ...
  "aurelia-framework": "1.1.4",
  "aurelia-loader-webpack": "2.1.0",
  "aurelia-dialog": "1.0.0-rc.1.0.3",
  "aurelia-pal-browser": "1.3.0",
  "bluebird": "3.5.0",
  ...
}

modal.html

...
<a>Some link</a>
...

I load the viewmodel with PLATFORM.moduleName("./modalVm").

It crashes as soon as it sees the <a> no matter if further arguments are applied.
When I comment out all the links it works again.

---

EDIT:
Note that I need to combine both loading methods, because I need access to the ctor of my viewmodel.

So the whole thing looks like this:

import { ModalVm } from "./modalVm";
const dialogModel = new ModalVm(...);
this.dialogService.open({ viewModel: PLATFORM.moduleName("./modalVm"), model: dialogModel })

[jods4]

Does it go away if you don't pass the model in this way?
Not saying it's a solution, just trying to understand what happens...

With moduleName("./modalVm") this module should not register (by accident) a <a> custom element.
Can there be another module interfering?

[Xceno]

Okay I figured out what was going on!
My webpack.config was messed up, so I loaded the same file for model and viewModel.

I have my dialog resources configured as globalResources in main with some redirects via ModuleDependenciesPlugin and screwed up the paths inside this.

Sorry for the hassle and thanks for your time!

[jholland918]

Why is this closed?

[jods4]

@jholland918 scroll above, up to the "close issue" event, look at the last comment:

Tracked in aurelia/templating#549

[jholland918]

@jods4 thanks, I didn't see it was tracked in aurelia/templating#549 :)

[jods4]

No problem.
It was moved because I identified the bug inside the Aurelia core libraries, it's not specific to aurelia-dialog.

[iiiyx]

Sometimes moduleId resolved by Webpack is a number and the workaround doesn't help in this case.

We have such DialogService's ensureViewModel method after compilation using Webpack:

DialogService.prototype.ensureViewModel = function (compositionContext) {
        if (typeof compositionContext.viewModel === 'function') {
            var moduleId = aurelia_metadata["a" /* Origin */].get(compositionContext.viewModel).moduleId;
            if (!moduleId) {
                return Promise.reject(new Error("Can not resolve \"moduleId\" of \"" + compositionContext.viewModel.name + "\"."));
            }
            compositionContext.viewModel = moduleId;
        }
        if (typeof compositionContext.viewModel === 'string') {
            return this.compositionEngine.ensureViewModel(compositionContext);
        }
        return Promise.resolve(compositionContext);
    };

compositionContext.viewModel is our viewModel described as:

import {ACLEditor} from 'core/acl/acl-editor';
PLATFORM.moduleName('core/acl/acl-editor', 'core');
...
this.dialogService.open({
            viewModel: ACLEditor,
            model: {
                ...
            },
        });

Then we go to this line:

var moduleId = aurelia_metadata["a" /* Origin */].get(compositionContext.viewModel).moduleId;

And it becomes moduleId === "474"

And then compositionContext.viewModel === "474"

Then we go to this line

return this.compositionEngine.ensureViewModel(compositionContext);

Which leads us to this method

CompositionEngine.prototype.ensureViewModel = function ensureViewModel(context) {
    var childContainer = context.childContainer = context.childContainer || context.container.createChild();

    if (typeof context.viewModel === 'string') {
      context.viewModel = context.viewResources ? context.viewResources.relativeToView(context.viewModel) : context.viewModel;

      return this.viewEngine.importViewModelResource(context.viewModel).then(function (viewModelResource) {
        childContainer.autoRegister(viewModelResource.value);

        if (context.host) {
          childContainer.registerInstance(aurelia_pal__WEBPACK_IMPORTED_MODULE_2__["DOM"].Element, context.host);
        }

        context.viewModel = childContainer.viewModel = childContainer.get(viewModelResource.value);
        context.viewModelResource = viewModelResource;
        return context;
      });
    }

Here we have context.viewModel === "474" at this line

return this.viewEngine.importViewModelResource(context.viewModel)

importViewModelResource leads us to this method:

ViewLocator.prototype.getViewStrategy = function getViewStrategy(value) {
    ...
    var origin = aurelia_metadata__WEBPACK_IMPORTED_MODULE_1__[/* Origin */ "a"].get(value);
    var strategy = aurelia_metadata__WEBPACK_IMPORTED_MODULE_1__[/* metadata */ "d"].get(ViewLocator.viewStrategyMetadataKey, value);

    if (!strategy) {
      if (!origin.moduleId) {
        throw new Error('Cannot determine default view strategy for object.', value);
      }
      strategy = this.createFallbackViewStrategy(origin);
    } else if (origin.moduleId) {
      strategy.moduleId = origin.moduleId;
    }

    return strategy;
  };

Here value is ACLEditor()
And method returns {moduleId: "474", viewUrl: "474.html"}, but viewUrl should be "core/acl/acl-editor.html".

How can I get through it?

[bigopon]

I see that you are using webpack, and already used PLATFORM.moduleName on the path, you can use the moduleId

import {ACLEditor} from 'core/acl/acl-editor';
PLATFORM.moduleName('core/acl/acl-editor', 'core');
...
this.dialogService.open({
            viewModel: 'core/acl/acl-editor',
            model: {
                ...
            },
        });

[iiiyx]

@bigopon thanks! It helped.