Skip to content
This repository was archived by the owner on Jan 22, 2021. It is now read-only.

Removed Iron Router dependency #47

Merged
merged 3 commits into from
Dec 24, 2015
Merged

Conversation

satyavh
Copy link
Contributor

@satyavh satyavh commented Oct 28, 2015

Replaced Iron Router dependency with meteorhacks:picker, which means you can now use this package with Flow Router as well:

Uses meteorhacks:picker for server side routing
emailForUrl helper supports both Iron Router and Flow Router
Updated readme

Bumped to v0.7.0 - I thought it was quite major ;-)

Satya van Heummen added 3 commits October 28, 2015 20:41
Replaced Iron Router dependency with `meteorhacks:picker`, which means
you can now use this package with Flow Router as well.
@satyavh
Copy link
Contributor Author

satyavh commented Oct 29, 2015

I was thinking, before you accept this PR, people might be using Iron Router. So if they do any IR related stuff in the routes, that won't work anymore and break their code.

So maybe we should do a check for IR, and if there's IR we use that to setup server-side routes. Then the update will be backwards compatible.

@johanbrook
Copy link
Contributor

So maybe we should do a check for IR, and if there's IR we use that to setup server-side routes. Then the update will be backwards compatible.

True. The change will cause unexpected things in people's actions, surely. I'd suggest we add Iron Router as a weak dependency (this package loads after Iron Router), and use a configuration flag:

Mailer.config({
  useIronRouter: true  // defaults to false
});

If set to true, we'll use Iron Router to create the email routes within this package, else Picker.

We'll advertise this in the README. Imo it's okay to introduce some breaking changes in 0.x versions.

Thanks again! 🎉


# if Flow Router
if FlowRouter?
baseUrl = Utils.joinUrl Mailer.settings.baseUrl, FlowRouter.path.call(FlowRouter, routeName, params.hash)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to store this in baseUrl here?

I also think we should have a fallback mechanism if the user has neither FlowRouter nor Iron Router installed. Like, doing a logger warning in the console.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ai, that line should be.

 # if Flow Router
if FlowRouter?
   Utils.joinUrl Mailer.settings.baseUrl, FlowRouter.path.call(FlowRouter, routeName, params.hash)

I'll change that. Sure, we can add a fallback mechanism

@satyavh
Copy link
Contributor Author

satyavh commented Nov 2, 2015

Or we can just check for

if Router?
  # do Iron Router stuff
else
  # do Picker stuff

that way we don't need to introduce any other dependencies and no config.

@johanbrook
Copy link
Contributor

Sounds good!

johanbrook added a commit that referenced this pull request Dec 24, 2015
Removed Iron Router dependency
@johanbrook johanbrook merged commit bfccb2f into lookback:master Dec 24, 2015
@johanbrook johanbrook mentioned this pull request Dec 25, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants