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

Allow import { model } from mongoose #3768

Closed
Elergy opened this issue Jan 16, 2016 · 14 comments
Closed

Allow import { model } from mongoose #3768

Elergy opened this issue Jan 16, 2016 · 14 comments
Labels
enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Milestone

Comments

@Elergy
Copy link

Elergy commented Jan 16, 2016

It's common when user wants to import variables from module without saving module into a variable, e.g.

let {Schema} = require('mongoose');

or

var Schema = require('mongoose').Schema;

but when I do this with model

var model = require('mongoose').model;

and try to create a model with imported function, I have an error

TypeError: Cannot read property 'modelSchemas' of undefined

Yes, model is a property of Mongoose.prototype, but Schema also is.
If documentation contains an instruction like

var Schema = mongoose.Schema,
...

it's possible to assume that 'model' will have similar behavior

@vkarpov15
Copy link
Collaborator

Supporting this behavior would involve a massive refactor of mongoose connections. Long story short, require('mongoose').model(); is different than var model = require('mongoose').model; model(); because the value of this in the former function call is the require('mongoose'); singleton, whereas it is the global object in the latter call. If you want a workaround, model.call(require('mongoose'), 'MyModelName', schema); should work fine. I'm not a huge fan of the fact that mongoose's top level functions are laden with side effects, but to get rid of that we'd also have to get rid of the mongoose.model() getter syntax, which would break a lot of people's code.

@Elergy
Copy link
Author

Elergy commented Jan 16, 2016

@vkarpov15 thanks for your answer!

There are 2 different cases:

  1. We know it's a bug, but fixing it takes a time for refactoring. Then we should keep the issue open
  2. We assert it is not a bug. Then we should close the issue

When we have the issue open, it can be fixed with breaking change (in new major release) or without breaking change (you know, everything can be refactored with backward compatibility, it just depends on effort). Someone (e.g. me) probably has time for it.

But when we mark the issue closed, we give contributors to know that pull requests with fixing this bug will be rejected, and someone doesn't even start to fix it.

What do you think about it?

@vkarpov15 vkarpov15 reopened this Jan 18, 2016
@vkarpov15
Copy link
Collaborator

Fair point, I'll keep this issue open. The workaround is easy, just ensure the functions have proper context. The eventual solution is no side-effects.

@breezewish
Copy link

+1

Currently this doesn't work:

import { model } from 'mongoose'

@vkarpov15 vkarpov15 modified the milestones: 4.5, 5.0 Feb 2, 2016
@vkarpov15
Copy link
Collaborator

Not expected to work in the immediate future because of side-effects in mongoose. No really good way to make mongoose.model() work as a getter/setter if you're using import { model } unless you use global state, which I'm loathe to do. Why don't you just use import 'mongoose'; ? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import

@jacob-israel-turner
Copy link

I'll +1 this.

import { model } from 'mongoose'

^ is a handy/attractive syntax.

jacob-israel-turner added a commit to jacob-israel-turner/mongoose that referenced this issue Mar 4, 2016
See Automattic#3768

To allow invocation of `model` in any context, I do a quick check to see if `this` within the invocation refers to the `mongoose` instance.  If it does not, `Mongoose.prototype.model` is called again with the proper context.
@jacob-israel-turner
Copy link

I created #3943 as a quick fix for this. If it's acceptable (until a better solution can be created), then I can write a test to go with the PR. If it needs revised at all, I'm more than willing to.

@vkarpov15
Copy link
Collaborator

I like the general idea, but tests fail in that pr, which is a deal breaker unfortunately

@jacob-israel-turner
Copy link

Yup, I did see that. I'll be playing with it tomorrow to see what's going
on.

On Sat, Mar 5, 2016, 17:45 Valeri Karpov notifications@github.com wrote:

I like the general idea, but tests fail in that pr, which is a deal
breaker unfortunately


Reply to this email directly or view it on GitHub
#3768 (comment)
.

@chrisbruford
Copy link

Came across this today after trying const { model } = require('mongoose');

needless to say it didn't work so back to let foo = mongoose.model('foo',fooSchema);

Be nice if someone has time for a fix but I can see why it's not high priority!

@ghost
Copy link

ghost commented Oct 9, 2018

So, to try and clarify, running

import { model } from 'mongoose';
//...
model('testModel', testModelSchema);

...exposes the model object to the global namespace, whereas

import mongoose from 'mongoose';
//...
mongoose.model('testModel', testModelSchema);

places the model in the scope of the already connected mongoose singleton object. Can some confirm this for me please.
Thanks

@vkarpov15
Copy link
Collaborator

@cr05s19xx that is generally correct. In the former, this is the global namespace, but in the latter this in model() refers to the mongoose singleton

@silverlight513
Copy link

If this error only occurs when we have this problem, it would be beneficial to print out the cause and fix so people don't end up having to google and coming to this issue like I did.

@vkarpov15 vkarpov15 modified the milestones: Parking Lot, 5.3.14 Nov 21, 2018
@vkarpov15 vkarpov15 added the developer-experience This issue improves error messages, debugging, or reporting label Nov 21, 2018
@vkarpov15 vkarpov15 changed the title Can not import 'model' from mongoose Throw error if doing import { model } from mongoose; or const { model } = require('mongoose') Nov 21, 2018
vkarpov15 added a commit that referenced this issue Nov 21, 2018
@vkarpov15 vkarpov15 added enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature and removed developer-experience This issue improves error messages, debugging, or reporting labels Nov 21, 2018
@vkarpov15 vkarpov15 changed the title Throw error if doing import { model } from mongoose; or const { model } = require('mongoose') Allow import { model } from mongoose Nov 21, 2018
@vkarpov15
Copy link
Collaborator

In 5.3.14, you'll be able to do import {model} from 'mongoose' and it will work properly. That is just for this one function though, we still need more work to support that for the rest of the Mongoose global API. We'll still warn against doing this and keep the section in the FAQ, but the model case is so frequent that we can fix it as a one-off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Projects
None yet
Development

No branches or pull requests

6 participants