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

Saving context in application is an anti-pattern #43

Open
geekdada opened this issue Nov 27, 2018 · 5 comments
Open

Saving context in application is an anti-pattern #43

geekdada opened this issue Nov 27, 2018 · 5 comments

Comments

@geekdada
Copy link

I found on some occasions, the Model being instantiated with the last request context, then I found this:

this.ctx = ctx;

You should never save ctx in application ever. This is my fix:

{
  getServer(ctx) {
    if (!ctx[SERVER]) {
      const { config, model: Model } = this;
      const model = new Model(ctx);
      ctx[SERVER] = new AuthServer(Object.assign(config, { model }));
      return ctx[SERVER];
    }
    return ctx[SERVER];
  }
}
@geekdada
Copy link
Author

Or probably, use app/extend/context.js to get AuthServer attached on ctx.

@Azard
Copy link
Owner

Azard commented Nov 28, 2018

ctx[SERVER] = new AuthServer(Object.assign(config, { model })); could create AuthServer every time, it's not performance.

@geekdada
Copy link
Author

I'm a little confused, then what's this for?

const model = new Model(ctx);

If creating a new instance every time when a request comes is a bad approach, maybe you shouldn't have made Model has ctx as parameters in the first place.

@geekdada
Copy link
Author

I found this PR oauthjs/node-oauth2-server#462

@rise0chen
Copy link

Has the problem been solved?

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

3 participants