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

Lodash global template settings #2281

Closed
dizlexik opened this Issue Sep 15, 2014 · 7 comments

Comments

4 participants
@dizlexik
Contributor

dizlexik commented Sep 15, 2014

I got burned a few days ago while trying to do a sync() on my database and getting the error SyntaxError: Unexpected token = originating within lodash's .template function. It turns out and some other module in my project is altering lodash's global templateSettings.interpolate value and since Sequelize is utilizing the global lodash object it was wasn't able to parse templates with the default interpolate regex. I know this is the other library's fault at the end of the day but wouldn't it make sense within Sequelize to either pass default options to each call of .template(text, data, [options]) or perhaps instantiate a separate lodash object to Util._ instead of using the global singleton?

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Sep 15, 2014

We just use the one we install via NPM. I assume it's isolated to that install.

@dizlexik

This comment has been minimized.

Contributor

dizlexik commented Sep 16, 2014

Well, not exactly. I'm using the same version of lodash in my project alongside Sequelize which is what ends up populating Utils._ when Sequelize requires it in utils.js. This project-level lodash also gets required in the other module that is overwriting _.templateSettings values which subsequently causes Sequelize to error out when using _.template. Does that make sense?

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Sep 16, 2014

Not sure we can do anything about that then. If lodash is somehow sharing anything between seperate installs it's kind of hard to do anything about that (i don't see why they would code it like that).

Even if you are using the same version, unless you are taking it from Sequelize they should be installed in two seperate places in NPM i believe.

In any case i'm not sure how we can use a seperate lodash object anymore than we already are.

@janmeier

This comment has been minimized.

Member

janmeier commented Sep 16, 2014

But can't we override the defaults by always passing an options object?

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Sep 16, 2014

@janmeier yeah i suppose we could.

@dizlexik

This comment has been minimized.

Contributor

dizlexik commented Sep 16, 2014

To test this you can create a project that depends on both lodash and Sequelize. After npm install you'll notice that node_modules/sequelize/node_modules does not contain a lodash directory. So if you do something like require('lodash').templateSettings.interpolate = 123 then require('sequelize').Utils._.templateSettings.interpolate will equal 123 and all Utils._.template calls will fail. It's a side-effect of node's module caching and npm's merging of identical dependencies.

@mickhansen mickhansen added bug dependency and removed Not a bug labels Dec 3, 2014

@BridgeAR

This comment has been minimized.

Contributor

BridgeAR commented Apr 27, 2015

@janmeier @mickhansen This has nothing to do with sequelize and it would be best to fix the issue in the library that alters the the function. So I would close this issue.
@dizlexik Would be so kind and open an issue in the problematic library? Because this is bad for all librarys that depend on the lodash function.

janmeier referenced this issue Jul 2, 2015

Merge pull request #4044 from appropos/master
Don't override lodash includes() function.

@janmeier janmeier closed this in 0c88023 Jul 26, 2015

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