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

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

Closed
ghost opened this issue Mar 10, 2015 · 111 comments
Closed

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

ghost opened this issue Mar 10, 2015 · 111 comments

Comments

@ghost
Copy link

ghost commented Mar 10, 2015

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
Copy link
Member

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
Copy link

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

@arunoda
Copy link

arunoda commented Mar 19, 2015

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

@splendido
Copy link
Member

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:

  • useraccounts:ir-routing
  • useraccounts:flow-routing

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

Lets keep in touch!

@arunoda
Copy link

arunoda commented Mar 19, 2015

sure. Thanks.

@orlaqp
Copy link

orlaqp commented Mar 19, 2015

+1 to support flow-router.

Is there any way I can help with it?

@splendido
Copy link
Member

...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
Copy link

+1 to flow-router & useraccounts!

@splendido
Copy link
Member

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
Copy link

@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
Copy link
Member

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
Copy link

It works! Thank you much @PhilippSpo!

@steveodom
Copy link

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
Copy link
Member

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

@splendido
Copy link
Member

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
Copy link
Member

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
Copy link
Member

...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
Copy link

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
Copy link

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
Copy link
Member

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
Copy link

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
Copy link

+100 for flow-router + useraccounts!

@Sivli-Embir
Copy link

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

@DenisGorbachev
Copy link

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

@jononomo
Copy link

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.

@abernix
Copy link

abernix commented Aug 4, 2015

Ok, I can absolutely see different packages being a benefit for 2.0, and I break stuff up into packages internally - I fully understand the light-weight aspect of it. My only defense for keeping it in a single package is for easier on-boarding of new users. Auto-detection is always nice for new users, and I really don't know how many routers there are going to end up being. It's not out of the question that a router will be "built-in" at some point.

But really, I was really trying to address the problem for 1.0 users though, assuming that 2.0 is a bit off. I guess this all depends on the timeline for a 2.0 branch. 😄 I feel like many people are actively switching to flow-router right now (as in, yesterday) so perhaps a short-term solution (which @jshimko basically has) would be nice. Additionally, I feel like having flow-router in a separate branch right now might be actively discouraging users from using this (very good) package. The use seems to be dropping on Atmosphere... or perhaps everyone is doing local clones? I know I am.

@abernix
Copy link

abernix commented Aug 4, 2015

@rclai, I think that's the right idea. Though, I do think any requirement to add a different package will obviously have to come in a 2.0.0 release, and that's fine. So for @tjmonsi, I think that change would be too large for a 1.0.0 release because it would be a breaking upgrade for those already using iron-router.

@splendido
Copy link
Member

I'm still of the idea that also a usraccounts:flow-routing will be necessary to provide users with routes configured out of the box with a very few lines...

The thing is that some internal template initialization should be addressed within FR triggers before actual rendering, and I think letting also this part to the developer would be too much complicated and not that intuitive...

@splendido
Copy link
Member

about taking IR stuff out of the current version: we can do it very quickly, but I wouldn't bump to 2.0 (which would be the correct thing to do btw...).

We might think about a minor bump including some warning in case AccountsTemplates.configureRoute is called without the useraccounts:iron-routing package installed...
Does it make sense?
I obviously like to get people easily out of the IR dependency as quick as possible ;-)

@rclai
Copy link

rclai commented Aug 4, 2015

Yes, I was thinking that as well, just wasn't sure if you were comfortable doing that (the minor bump).

@rclai
Copy link

rclai commented Aug 4, 2015

The problem with having routes configured for Flow Router is that it will tie it to a specific template engine, which Flow Router was specifically designed not to do (@arunoda correct me if I'm wrong).

How would you tackle that?

@splendido
Copy link
Member

That's a good point!
I've read about the Blaze/React alternatives, but I actually had no time to have a look at FR yet... :(

Perhaps in this case the rendering engine could be autodetected within the useraccounts:flow-routing package?
...and we're back to the previous point! But maybe it would be easier in this case?

@jshimko
Copy link
Member

jshimko commented Aug 4, 2015

Or we could just make the rendering engine a configuration option for people using useraccounts:flow-router.

@rclai
Copy link

rclai commented Aug 4, 2015

That would be cool. Can we figure that out later though, perhaps in a separate issue? I don't think that it stops us from doing what is actionable now (if you're in agreement), which is to de-couple the iron:router features to another package, and let core be only atTemplates and the user account-related config functionality. What do you think?

@abernix
Copy link

abernix commented Aug 4, 2015

@splendido is right and ease-of-use route-creation (i.e. useraccounts:flow-routing, etc.) will be needed. I'm all about simplicity and ease of getting up-and-running (as I'm about to make very clear) 😄.

Why not have a default configuration, with just one package, that just works? Pick a router and go for it (iron-router is great). Keep the code well compartmentalized within the package, but have it integrated in the package. Make the dependency on this router weak: true. Then, if someone wants to add an override package (useraccounts:flow-routing?), fine, it will take the place of the default configuration and it will force that router. None of the built-in functionality will run.

In the case that someone does add useraccounts:flow-routing then have it (for now) just use BlazeLayout. I think that's fine for the short term and covers (guessing here) 90-95% of the users. Then, as @jshimko says, make it a configuration option in the future (3.0?).

Overall, my feelings are...

  • Leave this 1.0 branch in the dust. So many people are already probably forked into this flow-router-integration branch, if they want to stay there, they can stay there. I believe it will keep working as long as it's dependency stays as the meteorhacks:flow-router version, right? Anyone using the official version will see that a new 2.0 branch is out that they need to make changes for.
  • Obey semantic versioning - Why make a warning? The warning is in the version number.
    • Change milestones on other non-iron-router/flow-router related stuff to a 3.0 milestone - get to it soon.
    • Make a new 2.0.0 release which implements the rest of what has been suggested everyone, as I tried to summarize above, and release it asap.

It just seems like this router thing is the most pressing thing for this package right now to keep it moving forward with a strong user-base. I'm a big proponent of this package over the alternatives, and again, I feel like usage is dropping because use is getting too complex. Keeping things agnostic is well and good, but on-boarding should be stupid easy (almost as easy as accounts-ui).

@splendido
Copy link
Member

I'll try to get IR stuff out into useraccounts:iron-routing tonight then!
... @jshimko: could you do the FR part on useraccounts:flow-routing? v1.11.1

@jshimko
Copy link
Member

jshimko commented Aug 4, 2015

@splendido Yep. :)

@splendido
Copy link
Member

ok guys, I've just pushed some commits to core to get rid of IR stuff
and then I've also initialized the iron-rounting repository.

Tomorrow I'll have a bit of testing and I'll try to add something about this new scenario into the README.
...a comprehensive Quick Start section seems to be missing.

If you're quicker than me, I'll happily accept PRs ;-)

@splendido
Copy link
Member

...please note that you now need to define routes after the usual AccountsTemplates.configure({}) block since routes are now defined on the fly and some configuration should be set in advance for everything to work correctly.

@splendido
Copy link
Member

The counterpart for FR should follow soon on flow-rounting

@rclai
Copy link

rclai commented Aug 5, 2015

@splendido this is great! I'm glad that after 100 comments on here things are now mobilizing 😄

@arunoda
Copy link

arunoda commented Aug 5, 2015

Good job guys.
I really like the no-route solution for user-accounts.

@rclai yes flow-router's idea is to decouple and use it with different layout engines. I think the idea is successful with since now we can use both Blaze and React with FlowRouter.
That's why I like a router independent version.

@splendido
Copy link
Member

@rclai I'm sorry you guys had to wait 100 comments before getting things moving on...
I really hoped v2.0 was out much earlier with the routing stuff taken out of core, but I've been away for more than a month and I'm having very little spare time right now.

Hopefully things will get better also thanks to contributions from the community...

@splendido
Copy link
Member

@arunoda I'm glad you like it!

@splendido
Copy link
Member

I've just pushed a first version of flow-rounting!
I had to change a bit of stuff also in core to handle a few things.., but I had no time to test anything :(

Please let me know in case you get a chance to play a little with the new packages and find some problem!

@tjmonsi
Copy link

tjmonsi commented Aug 6, 2015

@splendido Will try in a few, but how will it work with let's say, useraccounts:materialize?

@tjmonsi
Copy link

tjmonsi commented Aug 6, 2015

Hi @splendido

Had to use this with a simple project and here's what I got:

W20150806-01:19:59.757(-4)? (STDERR)          
W20150806-01:19:59.759(-4)? (STDERR) /home/dev/.meteor/packages/meteor-tool/.1.1.4.beln8f++os.linux.x86_64+web.browser+web.cordova/mt-os.linux.x86_64/dev_bundle/server-lib/node_modules/fibers/future.js:245
W20150806-01:19:59.760(-4)? (STDERR)                                            throw(ex);
W20150806-01:19:59.760(-4)? (STDERR)                                                  ^
W20150806-01:19:59.760(-4)? (STDERR) TypeError: Cannot set property 'defaultLayoutRegions' of undefined
W20150806-01:19:59.760(-4)? (STDERR)     at packages/underscore/underscore.js:848:1
W20150806-01:19:59.760(-4)? (STDERR)     at Array.forEach (native)
W20150806-01:19:59.761(-4)? (STDERR)     at _.each._.forEach (packages/underscore/underscore.js:105:1)
W20150806-01:19:59.761(-4)? (STDERR)     at Function._.extend (packages/underscore/underscore.js:845:1)
W20150806-01:19:59.761(-4)? (STDERR)     at Package (packages/useraccounts:flow-routing/lib/core.js:15:1)
W20150806-01:19:59.761(-4)? (STDERR)     at /home/dev/workspace/bellweather-project/.meteor/local/build/programs/server/packages/useraccounts_flow-routing.js:257:4
W20150806-01:19:59.761(-4)? (STDERR)     at /home/dev/workspace/bellweather-project/.meteor/local/build/programs/server/packages/useraccounts_flow-routing.js:264:3
W20150806-01:19:59.762(-4)? (STDERR)     at /home/dev/workspace/bellweather-project/.meteor/local/build/programs/server/boot.js:222:10
W20150806-01:19:59.762(-4)? (STDERR)     at Array.forEach (native)
W20150806-01:19:59.762(-4)? (STDERR)     at Function._.each._.forEach (/home/dev/.meteor/packages/meteor-tool/.1.1.4.beln8f++os.linux.x86_64+web.browser+web.cordova/mt-os.linux.x86_64/dev_bundle/server-lib/node_modules/underscore/underscore.js:79:11)
=> Exited with code: 8

and it still loads iron router dependencies because it tries to load useraccounts:core. It messes up with the already loaded FlowRouter.

@tjmonsi
Copy link

tjmonsi commented Aug 6, 2015

Here's what it is loading in my dependencies:

iron:controller            added, version 1.0.8
iron:core                  added, version 1.0.8
iron:dynamic-template      added, version 1.0.8
iron:layout                added, version 1.0.8
iron:location              added, version 1.0.9
iron:middleware-stack      added, version 1.0.9
iron:router                added, version 1.0.9
iron:url                   added, version 1.0.9
useraccounts:core          added, version 1.11.1
useraccounts:flow-routing  added, version 1.11.1

While I already have these:

// Using FlowRouter
    "kadira:flow-router@2.0.2",
    "kadira:blaze-layout@2.0.0",

I think I'll try forking useraccounts:core and useraccounts:materialize and maybe remove the dependencies on iron router, then just try to play with it (Might just plan on using FlowRouter as is to define the routes)

@tjmonsi
Copy link

tjmonsi commented Aug 6, 2015

Ok I got it working now,

Forked your useraccounts:core (just found out that the core dependencies on Iron Router was removed yesterday, used mine for now)

Then did this...

"useraccounts:materialize@1.10.0",
"useraccounts:flow-routing@1.11.1",

It is currently working... for now... will update in a few

@splendido
Copy link
Member

Sorry, I should have mentioned I still haven't published the new changes.
So, yes, to test the new packages you have to do it with local clones...

Then:

  • export PACKAGE_DIRS="path/to/local/clones/"
  • cd yourProject
  • meteor add useraccounts:materialize
  • meteor add useraccounts:flow-routing
  • meteor

should be enough to go, provided you have local clones for useraccounts:core, and useraccounts:flow-routing (for useraccounts:materialize the latest published version should be just fine)

@splendido
Copy link
Member

The useraccounts:flow-routing package is out!
Thank you all for the great support :-)

...please report problems in separate issues from now.

@splendido
Copy link
Member

...please try it out and in case you like it, star it both on atmosphere and github ;-)

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

No branches or pull requests