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

Static numbered routes makes Meteor fail to start #986

Open
PEM-- opened this issue Oct 30, 2014 · 16 comments
Open

Static numbered routes makes Meteor fail to start #986

PEM-- opened this issue Oct 30, 2014 · 16 comments
Assignees
Labels
Milestone

Comments

@PEM--
Copy link

PEM-- commented Oct 30, 2014

Here the pattern of reproduction:

meteor create route11
cd route11
meteor add iron:router

Edit route11.js with the following content:

console.log('Starting...');
Router.route('/', function() { this.render('home'); });
Router.route('/01');
Router.route('/10');
Router.route('/11');

Meteor fails to start and provides the following stack:

I20141030-21:39:05.473(1)? Starting...
W20141030-21:39:05.476(1)? (STDERR)
W20141030-21:39:05.476(1)? (STDERR) /Users/PEM/.meteor/packages/meteor-tool/.1.0.35.1fel3lv++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/dev_bundle/lib/node_modules/fibers/future.js:173
W20141030-21:39:05.477(1)? (STDERR)                                             throw(ex);
W20141030-21:39:05.477(1)? (STDERR)                                                   ^
W20141030-21:39:05.490(1)? (STDERR) Error: Handler with name '11' already exists.
W20141030-21:39:05.491(1)? (STDERR)     at MiddlewareStack._create (packages/iron:middleware-stack/lib/middleware_stack.js:30)
W20141030-21:39:05.491(1)? (STDERR)     at MiddlewareStack.push (packages/iron:middleware-stack/lib/middleware_stack.js:46)
W20141030-21:39:05.491(1)? (STDERR)     at Function.Router.route (packages/iron:router/lib/router.js:131)
W20141030-21:39:05.492(1)? (STDERR)     at app/route11.js:6:8
W20141030-21:39:05.492(1)? (STDERR)     at app/route11.js:8:3
W20141030-21:39:05.492(1)? (STDERR)     at /Users/PEM/tmp/route11/.meteor/local/build/programs/server/boot.js:168:10
W20141030-21:39:05.493(1)? (STDERR)     at Array.forEach (native)
W20141030-21:39:05.493(1)? (STDERR)     at Function._.each._.forEach (/Users/PEM/.meteor/packages/meteor-tool/.1.0.35.1fel3lv++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/dev_bundle/lib/node_modules/underscore/underscore.js:79:11)
W20141030-21:39:05.493(1)? (STDERR)     at /Users/PEM/tmp/route11/.meteor/local/build/programs/server/boot.js:82:5
@cmather
Copy link
Contributor

cmather commented Oct 30, 2014

Interesting. Not sure how this would happen. Can you debug a little in chrome and let us know what you find? that error check I think just checks for the existence of a property in an object (e.g. the path). So not clear to me from the example above why it might be duplicating.

@cmather cmather added the bug label Oct 30, 2014
@cmather cmather added this to the Requires Triage milestone Oct 30, 2014
@PEM--
Copy link
Author

PEM-- commented Oct 30, 2014

Hi @cmather, I've updated the reproduction scenario (forget to show that I only added one package).

Here is the list of package involved:

autopublish      1.0.1  Publish the entire database to all clients
insecure         1.0.1  Allow all database writes by default
iron:router      1.0.0  Routing specifically designed for Meteor
meteor-platform  1.2.0  Include a standard set of Meteor packages in your app

Pretty basic. There shouldn't be anything that mess with the routes.

I've checked Chrome dev tools's console. It displays nothing. In the Chrome browser, Meteor displays the same error as the CLI.

Even, if I remove autopublish and insecure, the problem still shows up. I could strip down meteor-platform but I think it would start to feel weird for our new users.

@cmather
Copy link
Contributor

cmather commented Oct 30, 2014

Well first thing to check is: does this issue happen with a new project. It might be a weird issue with how javascript deals with object property names (which is how we're determining if a route already exists). The code that's checking whether the handler already exists by name is in iron:middleware-stack here: https://github.com/EventedMind/iron-middleware-stack/blob/master/lib/middleware_stack.js#L29.

I'll try to take a closer look sometime soon too. Just giving you some extra tools to dig into it in case I run out of time to get to it before leaving for a trip today.

@tmeasday, do you see anything here that would cause the handler name to be duplicated based on the scheme illustrated in the first comment above?

@PEM--
Copy link
Author

PEM-- commented Oct 30, 2014

Humm.. Trying to debug it currently, but I'm a bit lost.

@PEM--
Copy link
Author

PEM-- commented Oct 30, 2014

OK, I see now where it hangs but do not know already what is the cause of it. When adding the 4th route, the current dictionary of routes contains 5 routes. Some previous steps have introduced undesirable routes. It should only contains 3.
capture d ecran 2014-10-30 a 23 56 51

@PEM--
Copy link
Author

PEM-- commented Oct 30, 2014

Actually, in this Array, the route 1 and 01 are exactly the same. They have been duplicated on previous steps.

Sorry guys, I can't go any further. It's too late in France for carrying on. I hope that I've lead you on the right track.

@tmeasday
Copy link
Contributor

Can confirmed that simply creating a project with

Router.route('/10');
Router.route('/11');

Causes the problem. It's not obvious to me digging through the source why it would happen.

@cmather
Copy link
Contributor

cmather commented Oct 31, 2014

I'm on my way to France but will take a look when I get back to SF :-) I suspect this has something to do with either: (a) javascript object properties (tom try creating a regular object with these property keys and see if it does anything weird) or (b) some conversion being applied before I add to the middleware stack by name.

On Oct 30, 2014, at 4:20 PM, Pierre-Eric Marchandet notifications@github.com wrote:

Actually, in this Array, the route 1 and 01 are exactly the same. They have been duplicated on previous steps.

Sorry guys, I can't go any further. It's too late in France for carrying on. I hope that I've lead you on the right track.


Reply to this email directly or view it on GitHub.

@inderpreetsingh
Copy link

I just updated my app to latest version of meteor (1.0) from 0.8.1 and I am facing the same issue.

@cmather cmather assigned tmeasday and unassigned cmather Dec 17, 2014
@tmeasday
Copy link
Contributor

As I suspected the problem is that the middleware stack uses an array as both an object and a array:

https://github.com/EventedMind/iron-middleware-stack/blob/master/lib/middleware_stack.js#L33
https://github.com/EventedMind/iron-middleware-stack/blob/master/lib/middleware_stack.js#L48

I always thought this was asking for trouble :)

Try this:

var stack = []
stack['10'] = '10'
stack.push('10')
stack

You'll see the problem :) JS helpfully casts '10' to 10 when we write ['10']. Thanks JS.

@cmather I think the answer is: don't do that.

@pem and @inderpreetsingh the work around is don't use route names that are numbers.

@tmeasday tmeasday assigned cmather and unassigned tmeasday Dec 17, 2014
@cmather
Copy link
Contributor

cmather commented Dec 17, 2014

If it's not an array we can't support anonymous wildcard params. Maybe that's an okay trade off.

On Dec 16, 2014, at 9:53 PM, Tom Coleman notifications@github.com wrote:

As I suspected the problem is that the middleware stack uses an array as both an object and a array:

https://github.com/EventedMind/iron-middleware-stack/blob/master/lib/middleware_stack.js#L33
https://github.com/EventedMind/iron-middleware-stack/blob/master/lib/middleware_stack.js#L48

I always thought this was asking for trouble :)

Try this:

var stack = []
stack['10'] = '10'
stack.push('10')
stack
You'll see the problem :) JS helpful casts '10' to 10 when use write ['10']. Thanks JS.

@cmather I think the answer is: don't do that.

@pem and @inderpreetsingh the work around is don't use route names that are numbers.


Reply to this email directly or view it on GitHub.

@tmeasday
Copy link
Contributor

I think you are thinking of something else (doing the same wacky thing for params I guess). This is the stack of routes.

In either case there's definitely another way to do it!

@cmather
Copy link
Contributor

cmather commented Dec 17, 2014

Oh oops. Well we could have a separate object that serves as a name index if we want.

On Dec 16, 2014, at 10:13 PM, Tom Coleman notifications@github.com wrote:

I think you are thinking of something else (doing the same wacky thing for params I guess). This is the stack of routes.

In either case there's definitely another way to do it!


Reply to this email directly or view it on GitHub.

@tmeasday
Copy link
Contributor

Totally :)

@cmather
Copy link
Contributor

cmather commented Dec 17, 2014

I tried to get this done quickly but ran into some problems. So I think I'll try to get a release out today and come back to this one.

@PEM--
Copy link
Author

PEM-- commented Dec 17, 2014

Thanks for the workaround. That's exactly what I did. String expansion is not a big deal after all 😉 And thank you twice for your package and you dedication to it. I'm just impressed by your work, guys 👏 (a french fan).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants