Optional Viewport Content

[davismj]

I'm submitting a feature request

• Library Version:
  1.2.1

Please tell us about your environment:

• Operating System:
  All

• Node Version:
  6.2.0

• NPM Version:
  3.8.9

• JSPM OR Webpack AND Version
  JSPM 0.16.32 | webpack 2.1.0-beta.17
  Both

• Browser:
  all

• Language:
  all

Current behavior:
If you do not specify content for a viewport, an error is thrown:

Invalid Route Config: Configuration for viewPort "side" was not found for route: "departments."

• What is the expected behavior?
  If content is not specified, the viewport should be emptied.

• What is the motivation / use case for changing the behavior?
  There are various menus and panes in the application that are routable but not always shown.

[davismj]

I recognize there are workarounds, specifically the EmptyViewModel method. Why not make this the default behavior?

[EisenbergEffect]

It could be done. Do you want to do it? 😄

[bigopon]

Some1 do this please. Sometimes this could be very useful. At the moment if i wanna leverage the routing with my own router view, i have to specify a null view, same with empty view model but it doest feel aurelia for me.

Thanks 👍

[davismj]

@EisenbergEffect yea, can you point me in the right direction

[balazsmeszegeto]

@davismj I'd consider doing it, but didn't find EmptyViewModel

[davismj]

@djensen47 suggested that we make a configurable default per viewport, which would default to an empty view model.

[jwx]

I've got a somewhat different suggestion: I'd like the viewports to be independent of each other so that a route could specify any number of the existing viewports. The non-specified ones would then either a) be cleared or b) keep the current content (depending on a setting on the route). If this is something anyone else would like I could put together a PR. That is, of course, if it's not considered a bad idea or principle or something. @EisenbergEffect?

[EisenbergEffect]

That seems to make sense. Maybe comment here with a thorough explanation of how it would work before submitting the PR. Then we can discuss the details and make sure we're all on board.

[davismj]

@jwx Thanks for your input here. As I tried to respond it got me thinking. Here's what I think is the right solution, especially in light of #526:

app.html

<!-- default="menu" specifies the default view port instruction for the router-view is 
  { moduleId: 'menu' } if not specified, the default view port instruction is { moduleId: null }, 
  which is an empty view port --> 
<router-view name="menu" default="menu"></router-view>
<router-view name="main"></router-view>

app.js

 configureRouter(config, router) {
  config.map([

    // navigating to 'empty' leaves the menu view port empty, since moduleId is null
    { route: 'empty', name: 'empty', viewPorts: { 
      menu: { moduleId: null },
      main: { moduleId: './home'}
    } },

    // navigating to 'keep' makes no change to the menu view port since it is not specified;
    // if there is no previous view port instruction, then the default view port instruction
    // is used
    { route: 'keep', name: 'keep', viewPorts: { main: { moduleId: './home'} } }

  ]);
}

[jwx]

@davismj Yeah, that's what my code is doing. (With some additions that I'll go into as soon as I get the time; hopefully later this evening.)

[jwx]

Here's what my code is currently doing:

When navigating to a new route, the route presents all accumulated viewports with the new route's viewPorts specification added onto them. In the new route's viewPorts property, if a viewport

• specifies a module, the viewport is updated with the module
• specifies null, the viewport is emptied
• is left out or specifies undefined, the viewport remains unchanged

In addition, the route has a new property, explicitViewPorts, which makes all viewports that are not existing in the route's viewPorts property to be treated as null and therefore emptied (instead of undefined and unchanged).

If a viewport should remain unchanged (it's undefined) and it doesn't have any previous value (typically when entering or refreshing the app), a new property on RouterConfiguration called defaultViewPorts (which has the same syntax as viewPorts on a route) is checked for a previous value (and then both null and undefined results in empty). So in my version, default viewports are setup in configureRouter and not in the router-view element's properties.

Is it thorough enough, @EisenbergEffect ? (There isn't really much more to my code, anyway.)

What do you guys think? Should I make a PR?

[davismj]

Actually, I think the RouterConfiguration approach is better, since it will keep all the route configuration logic in the same place.

specifies null, the viewport is emptied
is left out or specifies undefined, the viewport remains unchanged

I think this is going to be confusing for too many developers. What you're saying is the two routes below will have different behaviors:

  { route: 'empty', viewports: { 'main': null },
  { route: 'no-change', viewports: { 'main': undefined }

While I think this makes a great deal of sense, in the JavaScript world too many people are sloppy with their nulls and undefineds, using them interchangeably. I'd rather see a more explicit API like the one I described above.

  { route: 'empty', viewports: { 'main': { moduleId: null } },
  { route: 'no-change', viewports: { 'main': undefined } }

I tend to like this API because it the empty route reads like "put something in the main viewport, but that something is nothing." A corollary is that it opens the opportunity to make the API for the no-change route flexible:

  { route: 'empty', viewports: { 'main': { moduleId: null } },
  { route: 'no-change', viewports: { 'main': null } }

Though I think you're right that the most common use case will be unspecified:

  { route: 'empty', viewports: { 'main': { moduleId: null } },
  { route: 'no-change', viewports: { } }

Thoughts?

[jwx]

Well, as a general principle, if I have to chose, I tend to go for code that make sense rather than enables sloppy developers. But I'm old school and somewhat grumpy.

Still, while these two are indeed different

{ route: 'empty', viewports: { 'main': null }
{ route: 'no-change', viewports: { 'main': undefined }

both of these currently work and result in the same thing (empty)

{ route: 'empty', viewports: { 'main': null }
{ route: 'empty', viewports: { 'main': { moduleId: null } }

meaning that the developer can chose personal preference here.

I really don't like the use of null for unspecified since in JS/TS null is specific for nothing or empty and undefined is specific for hasn't been assigned. Also, I think using null for both empty and no change is more confusing since

{ route: 'empty', viewports: { 'main': { moduleId: null } }
{ route: 'no-change', viewports: { 'main': null } }

reads as "put a module in the main viewport, but that module is nothing" and "put nothing in the main viewport" respectively.

I really think it should be undefined, either specified or by omission, for no change and null, either on the viewport or the module, for empty.

[davismj]

I'm mostly convinced. The part where you lose me is

null, either on the viewport or the module, for empty.

While you have me sold that it's not good to introduce flexibility for sloppiness, I think along the same lines we are going to get sloppy devs getting unexpected behaviors. While I believe this is an important feature, I want to do my best to reduce incoming tickets.

Let's just make it null on the moduleId, and the following would throw an error as it does in the current implementation:

// this is an error
{ route: 'empty', viewports: { 'main': null }

I'm curious to hear from other people. It's always good to have a range of opinions.

[jwx]

@davismj So is it then

{ route: 'empty', viewports: { 'main': { moduleId: null } }
{ route: 'no-change', viewports: { 'main': undefined } }

where an omitted viewport would also mean no change? I'd have to add some code back in, but it's not that big of a deal and I think the syntax is good enough. (Even if it is a bit verbose for no apparent reason. 😉)

Thoughts from anyone else? @EisenbergEffect ?

[davismj]

Nice work!

[djensen47]

In the meantime, what's the workaround for specifying a viewport as empty?

[davismj]

@jwx Thanks again for your hard work on this. I've worked on your branch and have opened a PR here: #551. I've gotten rid of the explicitViewPort to reduce the complexity of the feature but other than that things are basically the same. I will pull this in shortly.

@djensen47 #551