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

Share one Shopify instance across Express app #191

Closed
tomblanchard opened this Issue Jun 6, 2018 · 13 comments

Comments

Projects
None yet
2 participants
@tomblanchard
Copy link

tomblanchard commented Jun 6, 2018

I'm developing a Shopify app which uses Express and this module for API calls and I've run into trouble sharing one Shopify instance across my routes. Here's a stripped down version of what I'm trying to achieve:

var express = require("express");
var Shopify = require("shopify-api-node");
var app = express();

// Declare at highest scope so all routes can reference it
var shopify;

app.get("/", (req, res) => {
  var { shopName, apiKey, password } = req.query;

  // Re-declare to a Shopify instance using data from query string parameter
  shopify = new Shopify({ shopName, apiKey, password });

  shopify.shop.get()
    .then((shop) => {
      return res.send(JSON.stringify(shop));
    })
    .catch(() => {
      return res.status(500).send("Something broke.");
    });
});

app.get("/other-route", (req, res) => {
  shopify.product.list()
    .then((products) => {
      return res.send(JSON.stringify(products));
    })
    .catch(() => {
      return res.status(500).send("Something broke.");
    });
});

app.listen(3000, () => {
  console.log("Example app listening on port 3000!")
});

That actually works like how I want it to:

What I'm struggling with is eventually all routes will be in separate files, how could I share that one Shopify instance between all files?

In my actual non-stripped-down app I'm using express-session, I tried storing the Shopify instance in that but that only takes JSON-serializable data so that doesn't work. Is there an Express-best practice I've missed when it comes to storing instances across routes/session?

Right now I'm creating a new Shopify instance for each route which isn't ideal, it works but eventually I'll run into problems with the API call limit, I'd prefer to use one Shopify instance across all of my routes with autoLimit: true and not have to worry about it, any help would be greatly appreciated.

PS: Thanks once again for this module, it's great.

@lpinca

This comment has been minimized.

Copy link
Collaborator

lpinca commented Jun 6, 2018

A possible option is to define the route handlers like this:

function otherRouteHandler(shopify) {
  return (req, res) => {
    shopify.product.list()
    .then((products) => {
      return res.send(JSON.stringify(products));
    })
    .catch(() => {
      return res.status(500).send("Something broke.");
    });
  }
}

and then use it as follows:

app.get("/other-route", otherRouteHandler(shopify));

If your Shopify instance is created at runtime and your routes depend on it, you probably also need to add the routes handlers at runtime after the Shopify instance is created.

@tomblanchard

This comment has been minimized.

Copy link
Author

tomblanchard commented Jun 6, 2018

@lpinca I just tried that out and realised the whole concept of storing the Shopify instance in a higher-scope variable is flawed. For example:

  1. Visit a URL like: http://localhost:3000/?shopName=your-shop-name&apiKey=your-api-key&password=your-app-password
  2. API call works
  3. Open new incognito tab
  4. Visit the URL: http://localhost:3000
  5. It shows the API call from the first URL in this list

So, the Shopify instance needs to be stored in the session, how can we achieve this?

@lpinca

This comment has been minimized.

Copy link
Collaborator

lpinca commented Jun 7, 2018

I don't follow, you only have to make sure to use the same instance and not reassign it. Using your code:

const express = require('express');
const Shopify = require('shopify-api-node');
const app = express();

let shopify;

app.get('/', (req, res) => {
  const { shopName, apiKey, password } = req.query;

  if (shopify === undefined) {
    shopify = new Shopify({ shopName, apiKey, password });
    app.use(createRouter(shopify));
  }

  shopify.shop.get()
    .then((shop) => {
      return res.send(JSON.stringify(shop));
    })
    .catch(() => {
      return res.status(500).send('Something broke.');
    });
});

app.listen(3000, () => {
  console.log('Example app listening on port 3000!')
});

function createRouter(shopify) {
  const router = express.Router();

  router.get('/other-route', (req, res) => {
    shopify.product.list()
      .then((products) => {
        return res.send(JSON.stringify(products));
      })
      .catch(() => {
        return res.status(500).send('Something broke.');
      });
  });

  return router;
}

This is untested but it should work. You can't store the instance in the session as that requires the object to be serialised and deserialised for every request. It's much more efficient to create a new instance every time in this case.

@tomblanchard

This comment has been minimized.

Copy link
Author

tomblanchard commented Jun 7, 2018

@lpinca Ah defining the other routes inside the "/" route is nice, I didn't know that was possible.

However, this method still depends on a higher-scope shopify variable, which has it's flaws. The way I need this app to work is every session starts from fresh, for example if I navigate to this URL in a new incognito window:

http://localhost:3000/?shopName=shop-one&apiKey=your-api-key&password=your-app-password

It'll display the data from API calls to the shop-one shop, now if I close this window and open a new incognito window and navigate to this URL:

http://localhost:3000

It'll still show data from the shop-one shop API call, when it should display an error because the query params weren't properly set. This is happening because when the first URL was visited, the higher-scope shopify variable was set, it's set in a global fashion and not per-session.

Do you see the issue?

@lpinca

This comment has been minimized.

Copy link
Collaborator

lpinca commented Jun 7, 2018

Yes, but I don't see the problem. Can't you check the session and use/set the shopify object accordingly?

If you need multiple Shopify objects based on a session value, you can keep a map of them.

@tomblanchard

This comment has been minimized.

Copy link
Author

tomblanchard commented Jun 7, 2018

@lpinca That sounds like a good way around it, I basically need a new Shopify instance for every new session.

Is this something you'd be able to provide an example of, or does this fall outside of the Shopify-api-node scope?

@lpinca

This comment has been minimized.

Copy link
Collaborator

lpinca commented Jun 7, 2018

I think it's out of the scope of this module, sorry.

@tomblanchard

This comment has been minimized.

Copy link
Author

tomblanchard commented Jun 7, 2018

@lpinca Thought so, no worries, thanks for the help Luigi. Do you happen to know of any other open source projects which use the pattern of storing instances inside session maps, I've never seen that and I'd love some inspiration on how to achieve it.

@tomblanchard

This comment has been minimized.

Copy link
Author

tomblanchard commented Jun 7, 2018

@lpinca Figured it out in the end:

var express = require("express");
var session = require("express-session")
var Shopify = require("shopify-api-node");

var app = express();

var shopifys = {};

app.use(session({
  secret: "gu2cQL7z2z4pKypm",
  resave: false,
  saveUninitialized: true
}));

app.use((req, res, next) => {
  if( !req.session.shopName || !req.session.apiKey || !req.session.password ) {
    req.session.shopName = req.query.shopName;
    req.session.apiKey = req.query.apiKey;
    req.session.password = req.query.password;
  }

  if( !req.session.shopName || !req.session.apiKey || !req.session.password ) {
    return res.status(500).send("Something broke.");
  }

  var { shopName, apiKey, password }  = req.session;

  if( !shopifys[req.session.id] ) {
    shopifys[req.session.id] = new Shopify({ shopName, apiKey, password });
  }

  return next();
});

app.get("/", (req, res) => {
  var shopify = shopifys[req.session.id];

  shopify.shop.get()
    .then((shop) => {
      return res.send(JSON.stringify(shop));
    })
    .catch((err) => {
      return res.status(500).send("Something broke.");
    });
});

app.get("/other-route", (req, res) => {
  var shopify = shopifys[req.session.id];

  shopify.shop.get()
    .then((shop) => {
      return res.send(JSON.stringify(shop));
    })
    .catch(() => {
      return res.status(500).send("Something broke.");
    });
});

app.listen(3000, () => {
  console.log("Example app listening on port 3000!")
});

Thanks again for the help, closing this now.

@lpinca

This comment has been minimized.

Copy link
Collaborator

lpinca commented Jun 7, 2018

Yes this is more or less what I was suggesting, the only problem is that it may create a memory leak if the session is destroyed and the corresponding instance is not removed from the map.

@tomblanchard

This comment has been minimized.

Copy link
Author

tomblanchard commented Jun 7, 2018

@lpinca Yeah I was thinking that, in the real world I wouldn't be relying on the express-session default store MemoryStore - probably redis. Do you have any ideas how you'd manage correctly removing the instance from the map when the session has "expired"?

@lpinca

This comment has been minimized.

Copy link
Collaborator

lpinca commented Jun 7, 2018

A dirty way would be to periodically compare the ids in the map with the ones in redis sess:* and remove the ones that no longer exists in redis.

@tomblanchard

This comment has been minimized.

Copy link
Author

tomblanchard commented Jun 11, 2018

FYI: In case anyone is interested in reading about this more in-depth, I blogged about this here:

https://tomblanchard.co.uk/storing-instances-in-express-sessions/

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