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

Don't overwrite globals when in a modular environment. #974

Merged
merged 1 commit into from
Mar 28, 2019

Conversation

garvank
Copy link

@garvank garvank commented Feb 11, 2019

Description

Requiring jQuery and _ without a var declaration were overwriting the global implementations of those libraries if they exist.

Because jQuery and _ can be customized and configured, it would be best to keep internal dependencies isolated.

I have a 2018 MBP and npm test does not run for me -- Karma simply times out. Since this falls back to the global versions, I don't foresee any tests failing. But please let me know if I'm missing a step to get tests running locally.

Checklist

  • Created tests which fail without the change (if possible)
  • All tests passing (npm test)
  • Extended the README / documentation, if necessary

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 65.011% when pulling 89ccf9f on garvank:develop into 590bc34 on gridstack:develop.

@garvank
Copy link
Author

garvank commented Feb 11, 2019

Hmm, happy to condense down to fewer lines to avoid a coverage decrease, but I think it's still worth it to not overwrite globals if we can.

@radiolips
Copy link
Member

Hey @garvank – We just removed lodash (thanks @adumesny !). Could you update this to only check for jQuery, then I'll merge? I agree that it should avoid overwriting global when possible.

@garvank
Copy link
Author

garvank commented Mar 28, 2019

@radiolips thanks for circling back to this! Updated as requested :)

@radiolips radiolips merged commit e84a7d9 into gridstack:develop Mar 28, 2019
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

Successfully merging this pull request may close these issues.

3 participants