Prevent context matching if the oldContext is null

[nathanhammond]

Fixes:

• Transitioning CSS class on multiple links to same route emberjs/ember.js#12419
• ember-transitioning-in CSS class applied incorrectly when passing ID instead of the whole model to {{link-to}} helper emberjs/ember.js#10989
• {{link-to}} classname ember-transitioning-in stuck emberjs/ember.js#10195
• param-only contexts are improperly considered equal when params differ #236
• META ISSUE: Routing fails to identify willBeActive for param-only changes. emberjs/ember.js#16275

[rwjblue]

Why is it not possible to test (can you explain)?

[nathanhammond]

This comment was wrong.

To trigger the issue the route actually needs to be processed asynchronously. The faked-in asynchronous behavior isn't enough to trigger it.

So, I would need to modify this line like so:

diff --git a/tests/router_test.js b/tests/router_test.js
index 07b2d69..cb3bafd 100644
--- a/tests/router_test.js
+++ b/tests/router_test.js
@@ -28,7 +28,15 @@ var scenarios = [
     getHandler: function(name) {
       // Treat 'loading' route transitions are synchronous
       var handler = handlers[name] || (handlers[name] = {});
-      return name === 'loading' ? handler : Promise.resolve(handler);
+      if (name === 'loading') {
+        return handler;
+      } else {
+        return new Promise(function(resolve) {
+          setTimeout(function() {
+            resolve(handler);
+          }, 1);
+        });
+      }
     },
     getSerializer: function(name) {
       return serializers && serializers[name];

And then approximately every async test fails since it is now running out of turn compared to the test suite.

[nathanhammond]

Also, I just found this gem:
https://github.com/tildeio/router.js/blame/c94df4be011665fcf3a4ad464413e927a9c648d3/lib/router/handler-info.js#L240-L254

[nathanhammond]

The problem with the old code is that it was simply comparing equality of the context. But the context could be unset for both old (if it is a loading transition) and new (always).

We need to check to see if there actually is a context, and if that context is shared. Only in that case is it a valid assumption to copy over params.

[rwjblue]

We should be specific here I think. For example, a 0 or ’’ (empty string) are probably fine...

[nathanhammond]

The bug is triggered by this process:

1. Generate a new TransitionState.
2. Start that TransitionState becoming resolved.
3. Before that transition has resolved (thus the async requirement for failure)...
4. ... attempt to check if the destination route is active using isActiveIntent.
5. Feed in the partially-completed transition to the "private" _state argument so that we compare against the ongoing transition.
6. This will trigger an attempt to read the context on oldHandlerInfo before it is assigned.
7. Then, because oldHandlerInfo (which isn't even finished being processed yet) doesn't have a context, it assigns oldContext to null.
8. This matches the prototype value from newHandlerInfo (obviously, sharing a prototype).
9. Then we clobber newHandlerInfo's params because we didn't count on this race condition.

[nathanhammond]

(Travis lost its mind. Restart the failing test one to see it actually fail.)

[rwjblue]

Restarted a few times, seems to not fail?

[rwjblue]

Does adding this guard allow us to delete this

router.js/lib/router/handler-info.js

Lines 240 to 254 in 2bfd8e5

	// this is bonkers, we require that `context` be set on on the
	// HandlerInfo prototype to null because the checks in
	// `NamedTransitionIntent.prototype.applyToHandlers` here
	// https://github.com/tildeio/router.js/blob/v1.2.8/lib/router/transition-intent/named-transition-intent.js#L76-L81
	// check of `oldHandlerInfo.context === newHandlerInfo.context` and assumes
	// that the params _must_ match also in that case.
	//
	// The only reason `oldHandlerInfo.context` and `newHandlerInfo.context` did not
	// match in prior versions is because if the context isn't set yet (on newHandlerInfo)
	// is because it inherits the `null` from the prototype vs `undefined` (on
	// the oldHandlerInfo).
	//
	// A future refactoring should remove that conditional, and fix the hand full of
	// failing tests.
	HandlerInfo.prototype.context = null;

If we can do that, we don't need the hasOwnProperty check here...

[nathanhammond]

We're still going to need hasOwnProperty for the scenario where: oldHandlerInfo.context === undefined && newHandlerInfo.context === undefined because neither explicitly set it.

It's also theoretically possible that use as a microlib would set context to a "meaningful" undefined value on the instance.

Regardless, I believe that hasOwnProperty is the correct check here.

[nathanhammond]

(See step 7 in #242 (comment))

[rwjblue]

Can you move this into a test of its own? Seems odd to just be an extra assertion in a somewhat unrelated test...

[nathanhammond]

• Test extracted.
• Removal of the "default" context from the prototype is added as an optional separate commit.

I opted to never explicitly set the context on a handler unless we actually received a context. Since we always compare identity but could have a primitive show up (undefined) we need to use hasOwnProperty to distinguish between an undefined context and an unset context.