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

Require deps like jQuery, postscribe #9

Closed
timwis opened this issue Mar 21, 2017 · 7 comments
Closed

Require deps like jQuery, postscribe #9

timwis opened this issue Mar 21, 2017 · 7 comments

Comments

@timwis
Copy link

timwis commented Mar 21, 2017

The js files appear to make use of jQuery and postscribe, but don't import the modules. Instead they assume the modules are globally available (in the window via a <script> tag import). If someone's using this module via require, they're probably not importing jquery and the like with <script> tags though.

package.json should include both of these modules as dependencies, and the files that use the modules should import them via require, ie. var $ = require('jquery')

@karissademi
Copy link
Member

karissademi commented Mar 21, 2017

True. Part of my reasoning for not including jQuery as a dependency was a note in on one of the USDS Standards releases referring to the decision to remove jQuery from their dependencies, because they received feedback that it was conflicting with other versions of jQuery some sites were already using. If you think that doesn't make sense for us, I can certainly add it in.

Agreed that postscribe should be added as a dependency, that was an oversight.

@timwis
Copy link
Author

timwis commented Mar 22, 2017

Ah, i see. That will come up when jQuery is global, like when you pull it in via a <script> tag, it gets attached to window.$ and window.jQuery. But when you use it via CommonJS (require('jquery')) it should only get attached to whatever variable you assigned it to, ie. a local var $ = require('jquery') and not pollute the global window space.

Although, tangentially, the main reason we need jQuery is for foundation's JS code, and foundation may require accessing it via the global window object. Not sure.

I think this falls into the "peer dependency" category of node modules, where you always require them when used in your code, but instead of putting them in dependencies, you put them in peerDependencies, and consumers install them separately.

Have you ever heard of foundation dropping jquery?

@karissademi
Copy link
Member

karissademi commented Mar 23, 2017

I'll do some testing re: including jQuery with require('jquery') and see if that's an issue for Foundation or not. Pretty sure Foundation won't be dropping jQuery any time soon.

@karissademi
Copy link
Member

Looks like in order for this to work, we'll also need to add jspm. I'm not entirely prepared to go down that rabbit hole at the moment, especially as adding jQuery as a <script> tag provides more flexibility to the project owner. Thoughts?

@timwis
Copy link
Author

timwis commented Mar 24, 2017

What suggests jspm would be need to be involved?

@karissademi
Copy link
Member

karissademi commented Mar 29, 2017

Upon closer inspection it does not appear that jspm is required after all. 👍

@karissademi
Copy link
Member

Fixed in bdba687.

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

2 participants