meteor-useraccounts / core

Meteor sign up and sign in templates' core functionalities
http://useraccounts.meteor.com/
MIT License
529 stars 281 forks source link

Don't hardly depend on iron:router - Flow Router Integration #308

Closed ghost closed 9 years ago

ghost commented 9 years ago

I'd like to use the useraccounts package with the flow-router. Right now it's not possible to get rid of iron:router when using the useraccounts package because of the hard dependency on it.

My suggested solutions to fix this are either:

  1. Make iron:router specific features optional. They are only available when the app uses iron:router. This can be achieved by making the dependency to iron:router weak and check the existence of Package['iron:router'] for the iron:router specific parts.
  2. Use polymorphism to support multiple routers. Create a common router interface object that will delegate the execution to the router that is included in the app. This would be iron:router, meteorhacks:flow-router or a "no router" implementation.
splendido commented 9 years ago

makes sense @Sanjo!

Probably solution 1. is the easier/quicker at the moment. Could you make a PR about this?

Solution 2. sounds better! But perhaps is something that could be better integrated into a useraccounts:routing package for v2?

steph643 commented 9 years ago

+1 for the ability to use flow-router instead of IR.

arunoda commented 9 years ago

@splendido let me know, if you need to any help? Just tag me or send me an email.

splendido commented 9 years ago

Hello @arunoda! Glad to see you here!

I really need to find some spare time to draw the skeleton for the new useraccounts@2.0 :(

I'm thinking to provide two independent routing packages:

without any doubts, it would be awesome having you involved in the package for the flow router ;-)

Lets keep in touch!

arunoda commented 9 years ago

sure. Thanks.

orlaqp commented 9 years ago

+1 to support flow-router.

Is there any way I can help with it?

splendido commented 9 years ago

...to get started even before I can manage to complete the new skeleton, we could play around on a new branch for useraccounts:core which is where all the routing support is now...

I've just created a new flow-router-integration branch! I'll happily accept PRs on this ;-)

Most of the routing set up is done in setupRoutes which now calls regular IR API.

Something else can be found here for the ensureSignedIn plugin...

Other Router.* calls could be found here and there though...

Let me know!

mallory-erik commented 9 years ago

+1 to flow-router & useraccounts!

splendido commented 9 years ago

Please all, les try out the current status for the Flow Router intergration!

I'd say the only way to do so is to locally clone useraccounts:core and checkout the flow-router-integration branch.

See also the Working Locally section on the README.md

Sivli-Embir commented 9 years ago

@splendido trying the flow router branch. I may have messed something up but I am getting the following error: TypeError: Cannot set property 'main' of undefined

#simple accounts config for now
AccountsTemplates.configure
  defaultLayout: 'mainLayout'
  homeRoutePath: '/'
  defaultContentRegion: 'main'
  showForgotPasswordLink: true

AccountsTemplates.configureRoute('signIn');
AccountsTemplates.configureRoute('forgotPwd');
AccountsTemplates.configureRoute('enrollAccount');
<template name="mainLayout">
    {{> Template.dynamic template=main}}
</template>

EDIT1: this happens on all routes. An easy example is the enroll account route.

EDIT2: the full trace:

TypeError: Cannot set property 'main' of undefined
    at AT.setupRoutes._.each.FlowRouter.route.action [as _action] (core.js:695)
    at route.js:59
    at runMiddleware (route.js:51)
    at route.js:47
    at AT.setupRoutes._.each.FlowRouter.route.middlewares (core.js:692)
    at runMiddleware (route.js:43)
    at Route._processMiddlewares (route.js:39)
    at Route.callAction (route.js:58)
    at router.js:282
    at Object.Tracker.nonreactive (tracker.js:560)
PhilippSpo commented 9 years ago

At the moment you need to set the defaultLayoutRegions property in your configure object:

AccountsTemplates.configure({
    defaultLayout: 'masterLayout',
    defaultLayoutRegions: {},
    defaultContentRegion: 'main'
});

But you are right that this should not happen, because {} should be the standard value for the defaultLayoutRegions property

Sivli-Embir commented 9 years ago

It works! Thank you much @PhilippSpo!

steveodom commented 9 years ago

I'm using the flow-router-integration branch locally with no problem. I'm having trouble when I try to deploy however. It seems to be using master. (it errors out on bad key defaultLayoutRegions, for example)

How would I use this branch in production? (I'm using Heroku with build pack meteor-buildpack-horse).

PhilippSpo commented 9 years ago

Thats a great question ...I had the same issue today an scalingo.io .. Anybody an idea how to handle this kind of stuff ?

splendido commented 9 years ago

where are you keeping the local clone of the repo? ...I'd say that if you put it under your project/packages folder meteor should pick-up that one instead of the official package.

As a tricky workaround, you can always change the version number with something like 1.8.1.1... It should be picked up with no problems

PhilippSpo commented 9 years ago

I cloned the useraccounts core package in project/packages, switched to the flow-router-integration branch and added it as a submodule to my project repo.. when the application gets bundled/built on scalingo (Heroku probably the same) it seems to ignore the submodule or uses the master branch instead of the flow-router-integration branch How did you do it @steveodom?

splendido commented 9 years ago

...just a silly pointer, the submodule is then pointing to the flow-router-integration branch? Make sure that if you clone the repo from scratch you are on the correct branch!

Sivli-Embir commented 9 years ago

Not with this branch but I have pushed a branch of this to production before. I believe I bumped the version number. Be sure your not using sym-links, they don't work (which makes sense but it is easy to forget).

steveodom commented 9 years ago

My clone of the repo is in /[myproject]/packages and is the right branch (I can see the flow-router changes). Meteor doesn't seem to be picking it up in production however.

I thought I'd be able to change the version number as was suggested with something like: meteor add meteor-useraccounts:core@1.8.1 but that says no such package. What am I doing wrong there?

PhilippSpo commented 9 years ago

I removed the package from the packages folder and did the following:

cd packages/
git clone https://github.com/meteor-useraccounts/core.git --branch flow-router-integration
cd ..
git add packages/core/

and then committed the whole thing. So in this case a didn't use a submodule and it seems to work fine.

Sivli-Embir commented 9 years ago

Ah! In your .meteor/packages do you have meteor-useraccounts:core? The version number is not required. I you don't have it there then it will pull the latest from atmo.

DenisGorbachev commented 9 years ago

+100 for flow-router + useraccounts!

Sivli-Embir commented 9 years ago

@DenisGorbachev the branch works quite well for now. User Accounts v2 is in the works.

DenisGorbachev commented 9 years ago

@Kestanous Thanks for the tip, I'll check it out.

jononomo commented 9 years ago

I would love to see iron-router removed as a dependency from this package. I followed all the instructions in this thread about using flow-router instead, but I'm still seeing iron-router get installed somehow. Every since switching to flow-router I feel like I actually understand what is going on in my app.

steph643 commented 9 years ago

@splendido, it seems Iron Router is now going downhill and Flow Router is almost officially backed by MDG (see here).

splendido commented 9 years ago

We'll be ready for a RC of 2.0 in a couple weeks! :-) there will be no routing support by default but packages useraccounts:ir-routing and useraccounts:fr-routing will be available as plugins to add routing support.

Any feedback about the names?

steph643 commented 9 years ago

Any feedback about the names?

Sounds good.

markudevelop commented 9 years ago

+1 @splendido sounds awesome everyone is switching to flow router (me included) and I'm really eager on the official 2.0 flow router + user accounts! :)

rclai commented 9 years ago

Maybe do useraccounts:iron-routing and useraccounts:flow-routing.

splendido commented 9 years ago

the poll is open! ;-)

Beelal commented 9 years ago

+1 @rclai

fightingtheboss commented 9 years ago

I agree with @rclai, the naming should be clear, no need for that level of brevity. Perhaps you can consider useraccounts:iron and useraccounts:flow as an alternative if you want to keep them short, but perhaps that's only clear for those already initiated.

splendido commented 9 years ago

Makes sense!

...so, at the moment it seems we're up to useraccounts:iron-routing and useraccounts:flow-routing

geritol commented 9 years ago

sounds epic :) Approx. when will be 2.0 ready?

splendido commented 9 years ago

I'd like to put out the first alpha version next week... But unfortunately I'm not having that much time to work on this.

splendido commented 9 years ago

...perhaps I should take some volunteer to work a bit on the initial take off ;-) Btw, there's an (almost empy) board on trello: see here

dandv commented 9 years ago

:+1: because the crowd says so and iron-router seems no longer actively maintained, despite some claims.

splendido commented 9 years ago

There were no updates from master to flow-router-integration since the last stable commit 90b947d1d605085d7bafb6eb2c0d6a2fa9a9e4e0.

I've now merged all newer modifications into flow-router-integration. Let me know if all works fine! ...otherwise we can revert back, or fix stuff

3even commented 9 years ago

So I've been trying to get the following boilerplate (https://github.com/aladine/meteor-boilerplate), which uses Flow Router and User Accounts, running but the app fails on start-up

=> Errors prevented startup:                  

   While building the application:
   lib/router/routes.js:9:2: Unexpected token )

Any help would be greatly appreciated.

splendido commented 9 years ago

I'd say you'd better report the issue on that repository, but it might be that someone here had a chace to play with it...

Btw, form the error itself, it seems is a sintax problem: even if looking at the first 9 lines of the file I couldn't spot anything weird....

PhilippSpo commented 9 years ago

@3even line1: FlowRouter.route(['/','/home]', { should be: FlowRouter.route(['/','/home'], {

splendido commented 9 years ago

I'm definitely blind :(

arunoda commented 9 years ago

This is regarding this comment/issue: https://github.com/meteorhacks/flow-router/issues/136

It would be great, if you could add some note on AccountsTemplates.setupRoutes() saying that to call it after the user have added routes. Otherwise, he/she might face issues as mentioned on the above post.

I hope this is true for both IR and Flow.

splendido commented 9 years ago

Thanks for pointing this out @arunoda! ...actually I already had other issues reported about route definition order, but I never realized this might be a problem with the pattern I've set up to define routes for UA.

I think the problem is AccountsTemplates.setupRoutes() is called at start up time (see here for the server and here for the client) within AccountsTemplates._init() (here and here). This obviously causes a delay in actual routes definition so that if someone have a catch all route defined in top level code after UA routes it actually gets defined before them ...and here is the problem!

The idea was to let devs modify UA routes possibly defined by some package they're using before actual definition, but since this deferral does not bring any evident advantage, I'd say the correct solution is to actually define routes straight away when AccountsTemplates.cofigureRoute is called.

Do you think it'll be possible to change a route's settings after its first definition? ...does this makes any sense?

arunoda commented 9 years ago

After the first definition, it's quite hard to change it. I assume you are looking to change the order, it's pretty hard.

I think, if you can define routes at AccountsTemplates.cofigureRoute, it feels logical.

splendido commented 9 years ago

I didn't meant the order, rather the name, path, and the like... But that's not a big point I guess.

I'll try to change the current behaviour: it should be a matter of moving just a couple of lines!

fightingtheboss commented 9 years ago

A few notes:

  1. It's hard to keep up with @arunoda but I've been running the branch successfully with the latest releases of flow-router (1.9.0) and flow-layout (1.4.0) spec'd in the package.json in my local clone. flow-layout 1.4.0 fixes a crucial issue when rendering views dynamically, but may not directly affect this package.
  2. I think any package that defines routes should mention that the order in which they're defined is significant, so both flow-router and useraccounts:core should make note in the documentation. I can help here if you'd like.
  3. I think defining the AccountsTemplates routes at the time of configuration is better, that way we can move those lines into the router where they probably belong. In fact, in that case, I'd suggest adding an alias for AccountsTemplates.configureRoute called AccountsTemplates.addRoute (or .defineRoute) because those, to me, imply configuration and definition.
splendido commented 9 years ago

thanks @fightingtheboss for your feed-back! I'll defenitely keep it in mind

Feel free to propose a PR to shed a light on this inside the Guide!

splendido commented 9 years ago

I've just merged some improovements by @PhilippSpo! See #428.