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

jQuery and what-input should be peerDependencies #11290

Closed
4 tasks done
ncoden opened this issue May 21, 2018 · 30 comments · Fixed by #11294
Closed
4 tasks done

jQuery and what-input should be peerDependencies #11290

ncoden opened this issue May 21, 2018 · 30 comments · Fixed by #11294

Comments

@ncoden
Copy link
Contributor

ncoden commented May 21, 2018

Description

I think that all current dependencies should be considered as peerDependencies for the following reasons:

  • When using the distribution files foudation.js, we expect the user to import jQuery by themself. To do this, we consider jQuery as an external in our build process and actually reproduce the peerDependency behavior.
  • Foundation register itself to jQuery and can be seen as a jQuery plugin.
  • Users do not expect jQuery to be specific to Foundation and want it to be imported only once, like most frontend dependencies.

peerDependencies is for dependencies that are exposed to (and expected to be used by) the consuming code, as opposed to "private" dependencies that are not exposed, and are only an implementation detail.

See:

Possible Solution

Move jquery and what-input to peerDependencies (but also devDependencies for local developement usages - see yarnpkg/yarn#1503).

Checklist (all required):

  • I have read and follow the CONTRIBUTING.md document.
  • There are no other issues similar to this one.
  • The issue title is descriptive.
  • The template is correctly filled.
@ncoden
Copy link
Contributor Author

ncoden commented May 21, 2018

Any opinion on this @DanielRuf @brettsmason ?

@DanielRuf
Copy link
Contributor

but also devDependencies for local developement usages - see yarnpkg/yarn#1503

What exactly is meant with this? In many cases the devDependencies are/will not installed.

@ncoden
Copy link
Contributor Author

ncoden commented May 21, 2018

@DanielRuf When we are locally building Foundation to run tests or start the documentation (for development purposes), we need peerDependencies to be installed too. Current Yarn (and maybe NPM) versions will not always install them so we need to declare them as both peerDependencies (production) and devDependencies (development).

@DanielRuf
Copy link
Contributor

Shouldn't be devDependencies sufficient in this case?

npm install (in package directory, no arguments):

Install the dependencies in the local node_modules folder.

In global mode (ie, with -g or --global appended to the command), it installs the current package context (ie, the current working directory) as a global package.

By default, npm install will install all modules listed as dependencies in package.json.

With the --production flag (or when the NODE_ENV environment variable is set to production), npm will not install modules listed in devDependencies.

NOTE: The --production flag has no particular meaning when adding a dependency to a project.

@ncoden
Copy link
Contributor Author

ncoden commented May 21, 2018

I would be devDependencies if we was only delivering dist files (i.e. foundation.min.js) to copy-paste or import from a CDN. But you can install Foundation from dependencies managers (npm i foundation-sites) and in this case we should have our dependencies checked. With peerDependencies we can expect the user to have jQuery installed and to be warned otherwise or if their jQuery version is not compatible with ours (<2.2.0).

@DanielRuf
Copy link
Contributor

I do not mean jQuery but the dev dependencies.

@ncoden
Copy link
Contributor Author

ncoden commented May 21, 2018

I do not mean jQuery but the dev dependencies.

I don't understand

@DanielRuf
Copy link
Contributor

#11290 (comment)

@ncoden
Copy link
Contributor Author

ncoden commented May 21, 2018

@DanielRuf

  • We need jQuery and motion-ui as peerDependencies instead of dependencies because we actually expect the user to install them. We cannot simply remove them because we still need the package managers to check for versions compatibilities and warn the user if they are missing.
  • We need jQuery and motion-ui as devDependencies too because we use them for tests/build/documentation and peerDependencies are not installed (even in development mode - Add a means to install package peer dependencies for development / testing yarnpkg/yarn#1503).

ncoden added a commit to ncoden/foundation-sites that referenced this issue May 22, 2018
peerDependencies is for dependencies that are exposed to (and expected to be used by) the consuming code, as opposed to "private" dependencies that are not exposed, and are only an implementation detail.

* We need `jQuery` and `motion-ui` as peerDependencies instead of dependencies because we actually expect the user to install them. We cannot simply remove them because we still need the package managers to check for versions compatibilities and warn the user if they are missing.
* We need `jQuery` and `motion-ui` as devDependencies too because we use them for tests/build/documentation and peerDependencies are not installed (even in development mode - yarnpkg/yarn#1503).

See foundation#11290
ncoden added a commit to ncoden/motion-ui that referenced this issue May 22, 2018
peerDependencies is for dependencies that are exposed to (and expected to be used by) the consuming code, as opposed to "private" dependencies that are not exposed, and are only an implementation detail.

We still need `jQuery` in peerDependencies instead of dependencies because we actually expect the user to install it. We cannot simply remove it because we still need the package managers to check for versions compatibilities and warn the user if it is missing.

See foundation/foundation-sites#11290
@ncoden
Copy link
Contributor Author

ncoden commented May 23, 2018

@DanielRuf So, what do you think ?

See #11294

ncoden added a commit that referenced this issue May 24, 2018
…dencies-11290

chore: move jQuery & what-input to peerDependencies #11290
ncoden added a commit to ncoden/foundation-sites that referenced this issue Jun 16, 2018
…er-dependencies-11290 for v6.5.0

b1b9408 chore: move jQuery & what-input to peerDependencies foundation#11290

Signed-off-by: Nicolas Coden <nicolas@ncoden.fr>
@josephcsible
Copy link

It made sense to do this for jQuery for all of the reasons you gave, but not for what-input. Most sites that use Foundation don't need what-input themselves, but now they all need to add it as a direct dependency of themselves anyway.

@ncoden
Copy link
Contributor Author

ncoden commented Aug 12, 2018

@josephcsible You are right, what-input is recommended but optional. However, all peerDependencies are optional and the NPM warning message can be safely ignored.

Removing what-input from peerDependencies would prevent us to ensure that the installed version of what-input is compatible with Foundation.

@josephcsible
Copy link

But why can't we just use our own version of what-input instead of requiring that whatever uses us have it? Unlike jQuery, there's no reason that it has to be the same as ours, right?

@ncoden
Copy link
Contributor Author

ncoden commented Aug 16, 2018

@josephcsible

We do not require what-input. It is perfectly safe not to install it. Since npm >= 3, npm does not install peerDependencies. This is why we also have these peerDependencies as devDependencies, to ensure we have them installed when running tests.

NPM is not quite suited to the front-end environement. There is optimizations to reduce the number of installed dependencies, but it is nothing compared to the Bower non-deterministic method that forced the user to select a single version of each package.

Peer dependencies are the best way to reproduce this behavior in NPM, and more than that we don't force anything to the user. No package is installed by default.

I found a good resume about it in the following article: https://survivejs.com/maintenance/packaging/managing-dependencies/

Peer dependencies (peerDependencies field in package.json) are dependencies that are required to use your package but the user should install them separately. They are usually given as version ranges.

The most common use case is plugins, for example a Babel plugins or a React component. You want to let your users decide which version of Babel or React they want to use to avoid installing multiple, and maybe incompatible, versions. That may be especially bad for frontend libraries.

Even if Foundation is not a plugin of what-input, we have the same requirements that peerDependencies resolves:

  1. Prevent to install multiple versions of the same package
  2. Check the compatibility of the package
  3. Allow not to install the package

@josephcsible
Copy link

@ncoden

We do not require what-input

Peer dependencies [...] are dependencies that are required to use your package

Those two sentences are my concern. I don't have any need for what-input, and neither does Foundation, but yarn complains loudly if I don't install it, since peer dependencies are considered required.

@DanielRuf
Copy link
Contributor

DanielRuf commented Aug 17, 2018

npm throws just a warning in this case that peerDeps are missing (eg adv). Still, what-input is not required to get Foundation Sites working as it progressively enhances Foundation afaik.

if I don't install it, since peer dependencies are considered required.

Do you mean the warning? It is just a warning.

@DanielRuf
Copy link
Contributor

DanielRuf commented Aug 17, 2018

I guess we should use optionalDependencies as peerDependencies describes a requirement (you need this to get it working).

https://docs.npmjs.com/files/package.json#optionaldependencies

https://docs.npmjs.com/files/package.json#peerdependencies

It is like a react plugin requiring react to work (peerDep). But it is not really meant as "this is not needed / required".

I think package.json is often used the wrong way. Let's test with optionalDependencies and see what Yarn and npm say then.

At least what-input.
And personally, StackOverflow answers are often not that right / exact (read the docs) ;-)

@ncoden
Copy link
Contributor Author

ncoden commented Aug 17, 2018

optionalDependencies is not what it sounds to be. The user cannot controls whether the optionalDependencies should be installed. This only means that if npm fails to install the dependency, the installation of your package will continue. It is intended to be used for packages that are only supported on a particular plateform (like fsevents).

@ncoden
Copy link
Contributor Author

ncoden commented Aug 17, 2018

Hi @zkat 👋. Could you help us on this. I searched for documentation about how to deal with optional dependencies in web-oriented packages but the only thing I found is from 4 year ago (https://blog.npmjs.org/post/101775448305/npm-and-front-end-packaging).

The problem

Foundation is a web-oriented package, a framework that provide distribution files to import in the HTML. It relies on jQuery (which is definitely a devDependency) and side-effects created by what-input.

  • We do not require what-input. Foundation works fine without it.
  • We do not import what-input directly.
  • We only need what-input for tests in browser (so it is at least a devDependency).

The question

  • Can what-input be considered as a peerDependency so the user can choose if it is installed and what version to install ?
  • Otherwise, should we care about the "browser environement" of our users and try to reduce the number of dependencies that would be finally required by its project ? Or is it "not our job" and should we not require what-input at all, preventing npm to detect when the user-installed version of what-input is incompatible with Foundation ?

I feels like there is a missing concept, like peerDependencies but not made for plugins (that "require" the main package to be installed by the top user to works) but for packages that can be (or not) installed by the end-user, that we may use directly or indirectly (side-effects in the browser), but that we can also ignore and still works well.

externalDependencies

External dependencies (externalDependencies field in package.json) are dependencies that are not required to use your package but are used for progressive enhancement. The user can install them separately.

@ncoden ncoden reopened this Aug 17, 2018
@DanielRuf
Copy link
Contributor

DanielRuf commented Aug 17, 2018

optionalDependencies is not what it sounds to be. The user cannot controls whether the optionalDependencies should be installed.

The developer should do this on his own / manually.

The fields are documented at https://docs.npmjs.com/files/package.json.
Also see npm/npm#14185

@DanielRuf
Copy link
Contributor

Created a small local test. jQuery was added as nomral dependency, what-input as optionalDependency.

├── jquery@3.3.1
└── what-input@5.1.1

--no-optional only installs jQuery:

➜  optional-test npm i --no-optional 
npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN optional-test@1.0.0 No description
npm WARN optional-test@1.0.0 No repository field.

added 1 package from 1 contributor and audited 2 packages in 1.198s
found 0 vulnerabilities

➜  optional-test npm ls              
optional-test@1.0.0 /Users/druf/projects/optional-test
├── jquery@3.3.1
└── UNMET OPTIONAL DEPENDENCY what-input@5.1.1

@DanielRuf
Copy link
Contributor

DanielRuf commented Aug 17, 2018

I guess our use case is not what can be solved with standard solutions (npm, package.json).

Generally other projects handle it like this: provide a npm / yarn install command which includes all needed peer / optional deps. For example npm install react react-dom (https://www.npmjs.com/package/react-dom) before they switched to create-react-app.

But peerDeps should be ok.
This is just a warning, no actual error:

npm WARN optional-test@1.0.0 requires a peer of what-input@^5.1.1 but none is installed. You must install peer dependencies yourself.

@DanielRuf
Copy link
Contributor

DanielRuf commented Aug 17, 2018

@josephcsible I see no error:

yarn install v1.9.4
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[-] 0/1(node:9459) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.
[3/4] 🔗  Linking dependencies...
[4/4] 📃  Building fresh packages...
✨  Done in 0.21s.

@DanielRuf
Copy link
Contributor

optionalDependencies is not what it sounds to be. The user cannot controls whether the optionalDependencies should be installed

Sure, with --no-optional

@DanielRuf
Copy link
Contributor

I guess either peerDeps was never meant for this (see the wording in the docs) or the wording is not really good and optionalDeps are always installed by default (which does not make them optional by default).

I would say we should use peerDependencies for now (but in this case jQuery will not be installed as before, also for contributors, which need a different install command then).

@josephcsible
Copy link

@DanielRuf yarn gives me this when I try it:

warning " > foundation-sites@6.5.0-rc.2" has unmet peer dependency "what-input@>=4.1.0".

@DanielRuf
Copy link
Contributor

warning " > foundation-sites@6.5.0-rc.2" has unmet peer dependency "what-input@>=4.1.0".

It's just a warning so it is not a problem ;-)

@DanielRuf
Copy link
Contributor

Warnings are totally fine in this case.

@glen-84
Copy link
Contributor

glen-84 commented Jan 15, 2019

See also:

yarnpkg/yarn#6487
yarnpkg/rfcs#105

@DanielRuf
Copy link
Contributor

Closing as the initial issue is resolved. The mentioned dependencies are now peerDependencies.

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

Successfully merging a pull request may close this issue.

4 participants