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

defaultAdapter behaviour #210

Open
moitias opened this issue Jun 11, 2014 · 3 comments
Open

defaultAdapter behaviour #210

moitias opened this issue Jun 11, 2014 · 3 comments

Comments

@moitias
Copy link

moitias commented Jun 11, 2014

So, the behaviour for defaultAdapter is still a bit odd in my view.

As it is, there seems to be three different ways for it to work:

  1. model.defaultAdapter = createAdapter(...) - this is as it currently works, as config.defaultAdapter isn't referenced anywhere
  2. model.config.defaultAdapter = createAdapter(...) - this is what the documentation advises
  3. model.config.defaultAdapter = [string] - this is what @mde described in fix crash on SQL datastores when using defaultAdapter #208

I'm guessing the idea is to work towards 3, but it's a bit unclear where would the options for the adapter then come from? 3 has the additional problem of being pretty opaque on when the adapter is actually created - it would be the only adapter not explicitly created by the user.

I'd be happy to sync the documentation and the code to any one of these. My personal preference would be just going for 1 - it seems to be a bit out of place to have the actual adapter in config and option 3 has the abovementioned problems.

@mde
Copy link
Contributor

mde commented Jun 11, 2014

The problem with 1 is that there's an existing convention that defaultAdapter is part of the 'model' config option provided in a Geddy app, and is a string value. Also I do generally prefer going through a method for setting and getting these things rather than accessing properties directly -- this works the same way at the individual-adapter level. Eventually I'd like to move to having the default adapter be a private, internal property, but I figured the first step was to provide a way to set it.

Number 2 is definitely wrong.

As far as the confusion about 3 -- it might make sense to lazy-create a defaultAdapter based on the config if it's not been set explicitly. The passed-in config for setDefaultAdapter would override any existing config values.

Right now the Geddy model-init code actually looks at the config.defaultAdapter and sets a .model property for each model definition it loads, instead of relying on the fallback (https://github.com/geddy/geddy/blob/master/lib/init/model.js#L92), but that behavior should change.

@moitias
Copy link
Author

moitias commented Jun 12, 2014

Ah, right, I'm approaching this from the non-geddy perspective so thinking of it in a very different way.

Looks like Geddy is still pretty intimately coupled with model, are you looking to decouple these or is model only supposed to be a supporting library for Geddy? Updating the behaviour of defaultAdapter would in any case need changes in both.

@mde
Copy link
Contributor

mde commented Jun 12, 2014

It shouldn't be so intimately coupled -- just that historically Geddy has been the biggest consumer. Model should be totally usable outside of Geddy (and I know people are using it that way). We can make whatever changes to Geddy's Model-init steps that make sense.

I was actually attempting to think of it purely from a code-design perspective. It's better if possible to have these adapters be set as internal, private properties, and set/get by methods, in the longer-term.

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

2 participants