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

MiddlewareStack duplicate handler issue on Microsoft Edge & Chrome Canary #1513

Closed
nathan-muir opened this issue Feb 17, 2016 · 26 comments
Closed

Comments

@nathan-muir
Copy link
Contributor

@tmeasday Not sure if you're still maintaining this. This is kind of related to #986 which you commented on. Also relates to #1270, and #1328.

This example now fails in the Microsoft Edge browser. (& Also in Chrome Canary w/ Experimental Javascript On.)

Why:
Because MiddlewareStack does some funky things with function names and uniqueness.

In Microsoft Edge, we now have new behaviour with function names & object literals:

var myObj = {
  // Note, function is anonymous.
  onBeforeAction: function() {} 
};
console.log(myObj.onBeforeAction.name)
// in Chrome/Firefox: ""
// in Edge: "onBeforeAction"
// in Chrome Canary + Experimental Javascript: "onBeforeAction"

That's right, anonymous functions now get names in object literals, even without using es6 object syntax.

How:

  1. Because MiddlewareStack._create enforces a 'unique' mapping of Handler's names.
  2. Handler automatically "picks" a name for any-named function

Proposed Solution:

I can't find any code in iron-router that even uses any of the name-related API's. (findByName, insertBefore and insertAfter).

So there are really two options:

  1. Add some option / flag / type that bypasses names in MiddlewareStack. (Maintaining existing API's if someone is using it?)
  2. Just remove anything name related from MiddlewareStack, and bump a major version. Pull Request
@nathan-muir nathan-muir changed the title MiddlewareStack using names is causing breakage on Edge MiddlewareStack issue on Microsoft Edge Feb 17, 2016
@nathan-muir nathan-muir changed the title MiddlewareStack issue on Microsoft Edge MiddlewareStack duplicate handler issue on Microsoft Edge Feb 17, 2016
@MiroHibler
Copy link

I can confirm this is also happening in the latest version of Chrome Canary (currently v50.0.2653.0 but it started in v50.0.2651.0 I believe).

@nathan-muir
Copy link
Contributor Author

Thanks @MiroHibler, I could reproduce in Chrome Canary too, but only after enabling "
Experimental Javascript" in chrome://flags/#enable-javascript-harmony

@chrisbutler
Copy link
Contributor

@nathan-muir is this definitely the fix? iron-meteor/iron-middleware-stack#8

@nathan-muir
Copy link
Contributor Author

@chrisbutler It's a fix (I've currently deployed for my apps);

Use at your own discretion, iron-router is a bit of a tangle though (to me), so I'm hoping to get someone more knowledgeable to review.

@tmeasday
Copy link
Contributor

Hi @nathan-muir -- I am not maintaining IR at this point. I'm not sure what the current status of maintenance is. @cmather?

@sylido
Copy link

sylido commented Mar 7, 2016

I can verify this behavior on Version 51.0.2669.0 canary (64-bit) without touching the default setting for Experimental Javascript (disabled). Removing the second onBeforeAction hook fixes the problem.

@nathan-muir nathan-muir changed the title MiddlewareStack duplicate handler issue on Microsoft Edge MiddlewareStack duplicate handler issue on Microsoft Edge & Chrome Canary Mar 8, 2016
@lorensr
Copy link

lorensr commented Mar 9, 2016

Looks related: #1520

I do have multiple global onBeforeActions, if that's the problem

@nathan-muir
Copy link
Contributor Author

@lorensr yeah, that would be it

@noosphere2
Copy link

Receiving this error in Chrome canary 51.0.2673.0:

middleware_stack.js:31 Uncaught Error: Handler with name 'route' already exists.

From a simple:

Router.route('/aroute', function() {});

Until @nathan-muir's code is reviewed and pulled into main repo, the error can be subdued by forcing each route to be named:

Router.route('/aroute', {name: 'aroute'}, function() {});

@maxfi
Copy link

maxfi commented Mar 11, 2016

Thanks @Zlot. That's fixed it for me on Chrome Canary 51.0.2673.0 (64-bit) as well.

tmeasday added a commit to iron-meteor/iron-middleware-stack that referenced this issue Mar 24, 2016
In newer browsers (chrome canary, IE11 as of this writing) this gets set automatically based on for instance object property names. This leads to lots of problems with duplicate names

iron-meteor/iron-router#1513
@tmeasday
Copy link
Contributor

Fixed: meteor update iron:middleware-stack.

@markreid
Copy link

markreid commented Jun 1, 2016

I understand there's a resolution for this but it may be worth noting that it's impacting Chrome 51.x which is now stable.

@brewhk-dev
Copy link

Running meteor update iron:middleware-stack didn't work for us. Instead, we had to ensure all routes are named - that fixed it.

@tmeasday
Copy link
Contributor

tmeasday commented Jun 8, 2016

@brewhk-dev what version of I:ms did you end up with?

@tmeasday
Copy link
Contributor

tmeasday commented Jun 8, 2016

I suspect I need to do a release of iron router that explicitly depends on this fixed version.

@brewhk-dev
Copy link

Our app is running on METEOR@1.1.0.2, with the follow iron:* versions:

iron:controller@1.0.8
iron:core@1.0.11
iron:dynamic-template@1.0.8
iron:layout@1.0.8
iron:location@1.0.9
iron:middleware-stack@1.1.0
iron:router@1.0.9
iron:url@1.0.11

Chrome version - 51.0.2704.84 (64-bit)

But simply updating the package didn't work for us, we had to make sure the routes are named.

@tmeasday
Copy link
Contributor

tmeasday commented Jun 9, 2016

@brewhk-dev --- that is very surprising to me. Can you provide a simple reproduction of the problem?

@tmeasday
Copy link
Contributor

tmeasday commented Jun 9, 2016

Ok, released iron:router@1.0.13

@levino
Copy link

levino commented Jun 9, 2016

Wont fix withiron:router@1.0.13 And I cannot use names easily as I have multiple "onBeforeActions". How would I give names to the routes then?

@tmeasday
Copy link
Contributor

tmeasday commented Jun 9, 2016

Wont fix withiron:router@1.0.13

What does that mean?

I don't understand how naming routes could fix this problem if it still occurs with that version. It removed code that looks at names.

@levino
Copy link

levino commented Jun 9, 2016

Edit: Removing this. Error on my side. Fix looks good.

@wbashir
Copy link

wbashir commented Jun 9, 2016

@tmeasday I tried adding this fix but with my Meteor version 1.1.0.2 I am running into an issue adding dependency packages.


$ meteor add iron:router@1.0.13
 => Errors while adding packages:             

While selecting package versions:
error: unknown package: isobuild:isopack-2
Required by: iron:dynamic-template 1.0.12
Required by: iron:layout 1.0.12
Required by: iron:router 1.0.13
Required by: iron:router 1.0.13

Any ideas?

@tmeasday
Copy link
Contributor

tmeasday commented Jun 9, 2016

Looks like you may be on an old version of Meteor?

danlg pushed a commit to danlg/lgen that referenced this issue Jun 10, 2016
@CarlQLange
Copy link

Yep, updating middleware-stack worked for me. Nice job @tmeasday and all IR contributors :)

harshjv added a commit to harshjv/question_tool that referenced this issue Jun 13, 2016
Before this, console shows error that;

    Uncaught Error: Handler with name 'route' already exists.

while using Chrome 51 and this (iron-meteor/iron-router#1513) also shows that it also occurs in Chrome 50+
@martriay
Copy link

Just like @brewhk-dev, naming the routes worked for me while updating didn't

@wbashir
Copy link

wbashir commented Jun 14, 2016

@tmeasday I am indeed on an older version of Meteor and ended up updating the middleware-stack

vjrj added a commit to comunes/bebes-robados that referenced this issue Jun 21, 2016
ivansglazunov added a commit to meteor-shuttler/meteor-shuttler that referenced this issue Jun 24, 2016
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

No branches or pull requests