Polymer / polymer

Our original Web Component library.
https://polymer-library.polymer-project.org/
BSD 3-Clause "New" or "Revised" License
22.03k stars 2.02k forks source link

Yarn Flat Mode Attribute in Package.json breaks projects #4823

Closed ChadKillingsworth closed 6 years ago

ChadKillingsworth commented 7 years ago

I fully understand the desire for flat dependencies and I think Polymer can be opinionated about requiring flat mode for apps. However, when a dependency has the flat: true in it's package.json, it forces any project which uses it into flat mode. This breaks all my existing projects.

my-app
 |- dep-1
 |- dep-2 // flat: true here forces the entire project into flat mode
 |- dep-3

This parameter is already ignored by npm so effectively what this parameter does is prevent developers from using yarn unless they use it exactly how the Polymer Project intends. And since flat mode and separate client/server dep folders is extremely non-standard, the end result is this may very well fracture the development space and be every bit as divisive as bower was.

With flat: true specified in Polymer core or in any other dependency, my choices are:

1 . Abandon yarn and go back to npm so that my projects can bypass the flat mode requirement.

  1. Republish every dependency I use to an internal npm repository. This is a maintenance nightmare.

As I understand it the problems flat is attempting to solve are:

I recommend documenting the flat mode requirement and having the Polymer provided build tooling check for it, but removing the flat: true property from any npm-published library intended to be used as a dependency.

rictic commented 7 years ago

Do you have more info on the kind of conflicts that you're seeing in your projects? One thing we've seen in other projects is that tooling-based dev dependencies introduce many conflicts, but there's no need for tooling to be flat. One pattern we're exploring is to make a tools directory with its own package.json.

ChadKillingsworth commented 7 years ago

The problem is less about causing actual breakages, and more that developers will simply switch back to npm to avoid the problem. I've already been sent a message by a dev:

Hey check out this Polymer 3 proof-of-concept. I can't get it to work with yarn so just use npm.

rictic commented 7 years ago

Chatted on slack with @ChadKillingsworth and @justinfagnani, riffing on some ideas @FredKSchott has been talking about too.

We have two constraints:

The core constraints listed above can be fulfilled with something less strict than flat. Let's call the first constraint unique and the second constraint webLoadable.

A package can have at most one transitive dependency on any unique package. As a result, the package manager, when installing, must place unique packages in./node_modules/$packageName, never in a nested node_modules directory.

A webLoadable package must be a sibling of all of its direct dependencies. So if foo is webLoadable and depends on bar then for every version of foo that the package manager creates, ../bar/ in that foo directory must point to the directory of an installed bar package that matches that foo's version constraints on bar.

These two constraints express our needs, and force the minimum amount of constraints on the rest of a package's dependencies. Currently flat causes a number of issues with projects in existing node projects, as installing a single flat dependency treats all of a projects' transitive dependencies and dev-dependencies as both unique and webLoadable, even though those constraints are often unnecessary (e.g. build-time tooling, and dependencies that aren't web compatible and only work in the browser after bundling).

flat might still be useful even with these constraints at the application level, for projects that are disciplined about their size and want to get early warning about code duplication.

hotforfeature commented 7 years ago

What about using npm peerDependencies? https://nodejs.org/en/blog/npm/peer-dependencies/

This would require elements to list their dependencies as peers instead of direct dependencies.

package.json

{
  "name": "@polymer/paper-button",
  "version": "3.0.0",
  "peerDependencies": {
    "@polymer/polymer": "^3.0.0"
  }
}

The downside to this is that a user would need to manually install the dependencies, of which npm would warn them about.

npm install --save @polymer/paper-button
npm ERR! peerinvalid The package @polymer/paper-button does not satisfy its siblings' peerDependencies requirements!
npm ERR! peerinvalid Peer @polymer/paper-button@3.0.0 wants @polymer/polymer@^3.0.0

npm install --save @polymer/polymer

The above would result in the following dependency tree:

├── @polymer/paper-button@3.0.0
└── @polymer/polymer@3.0.0

Unique? Yes, as long as elements declare each other as peer dependencies. If another element requires @polymer/paper-button, npm will resolve the best dependency match. If there's a version mismatch where two siblings require incompatible versions, npm will spit out an error in the console.

Web Loadable? Yes. All elements and their dependencies are installed by the app and exist in the top tier node_modules directory. No element installs its own nested dependency tree.

Another benefit is that this uses npm directly (no third party tool such as yarn) and it allows nested dependencies with other non-frontend npm modules and build tools.

Considering the amount of developer intervention that's needed to make flat node modules work with any other build tool, I'd consider the install step to be minimally intrusive.

justinfagnani commented 7 years ago

This is a general problem with modules, not just with Polymer, so in effect all dependencies would have to be peer dependencies, which mostly defeats the purpose of a package manager.

hotforfeature commented 7 years ago

But aren't peer dependencies essentially what Polymer 3 elements are trying to accomplish in the npm ecosystem? Elements don't import '@polymer/paper-button', they import '../@polymer/paper-button'

To me that's the element asserting that it's requesting a dependency that it does not have control of. It's expecting a sibling dependency and not a child dependency right?

For all front-end polymer elements, yes their dependencies should be peer dependencies since the convention is to require by path up to a sibling folder. However, this doesn't mean that all dependencies in the node_modules tree have to be peer. We can still get the benefit of nested dependency trees for other modules that are not importing each other by path.

Say for example, we have build tool that uses moment for dates, and our element is also using moment, but a different version. The packages would be

build-tool package.json

{
  "name": "build-tool",
  "dependencies": {
    "moment": "^1.0.0"
  }
}

build-tool.js

const moment = require('moment'); // Version 1

my-element package.json

{
  "name": "my-element",
  "peerDependencies": {
    "@polymer/polymer": "^3.0.0",
    "moment": "^2.0.0"
  }
}

my-element.js

import '../@polymer/polymer.html';
import '../moment/moment.min.js'; // Version 2

The node_modules tree would be

├── @polymer/poylmer@3.0.0
├── moment@2.0.0
├── my-element@1.0.0
├── build-tool@1.0.0
  └── moment@1.0.0

Using yarn with --flat is essentially forcing the entire node_modules tree to be "peer dependencies" instead of nested dependencies, so what's the real difference?

ChadKillingsworth commented 7 years ago

Does peerDependencies actually enforce a same level install? It's certainly not documented like that: https://docs.npmjs.com/files/package.json#peerdependencies

hotforfeature commented 7 years ago

Npm doesn't enforce or install any peers since npm v3. All it does is issue a warning to developers that essentially says

Hey! This @polymer/paper-button thing you have needs @polymer/polymer as a sibling and you haven't installed it yet. Pls fix.

It'll issue a warning nonstop each time you use an npm command until you install the missing dependency.

The upside is that since they're installed at the top level by the app, they're ensured to always be siblings and resolve paths correctly to each other.

The downside is that some elements may have a lot of peer dependencies (such as polymer, paper-styles, iron-behaviors, etc), and the user will have to npm install each peer dependency manually.

This is a big downside, but I think it's a better step in the right direction of using the npm ecosystem in the way it was designed without introducing third-party tools (yarn) or radically different npm workflows (flat installs).

We're early in the process, so the install hassle problem could be remedied with tooling or a new npm feature to auto-install peer dependencies. Something like npm install --peers --save @polymer/paper-button.

ChadKillingsworth commented 7 years ago

Ahh right - you are required to have them as direct dependencies of your app.

Manually installing all the dependencies is no worse (actually I view it as better) than maintaining resolutions with flat mode.

hotforfeature commented 7 years ago

Right, though that problem is way easier to fix than potential problems with flat mode and all the existing npm modules out there that would have issues with flat.

The ability to auto-install peer dependencies wouldn't be hard to add to npm under a flag and completely alleviate the install pain. Npm could even generate a warning such as

npm install --save @polymer/paper-button
npm ERR! peerinvalid The package @polymer/paper-button does not satisfy its siblings' peerDependencies requirements!
npm ERR! peerinvalid Peer @polymer/paper-button@3.0.0 wants @polymer/polymer@^3.0.0
npm ERR! Run npm install --peers to install missing dependencies.

npm install --peers

As an interesting side note, peer dependencies are how Angular ensures only one version of their packages are installed as well as only one version of Rxjs and Zone.js

zkat commented 7 years ago

hi~

we've got a thread going over at https://github.com/package-community/discussions/issues/2 about this issue.

For those who don't want to read through the Dostoyevsky book that thread's become, here's the tl;dr of what my team's planning on implementing:

  1. a new type of dependency, called an asset dependency, which will install into an assets/ directory, distinct from node_modules
  2. assets are guaranteed maximally flat, and existing separate means node tooling isn't broken by spurious forced flattening that it was never developed to support in the first place
  3. we're going to port Bundler's tree-flattening algorithm, which is pretty good at finding flat trees without requiring manual resolution. This is probably going to be much better than bower's algorithm (and, tbh, probably Yarn's) at finding flat trees without user involvement
  4. when all else fails, we're going to have a bower-style (but not bower-identical) resolutions definition in package.json so you can force a package into a version
  5. assets will be additionally written into package-lock.json, so you'll have reproducible builds
  6. all of this will be built in to npm itself, with the same stuff that's shipped out by default with node.js (once it lands)

tl;dr:

$ npm i --save-asset @polymer/example
$ echo '<script type="module" src="/assets/@polymer/example/example.js"></script><example></example>' \
  > index.html
$ npx serve
...(open http://localhost:5000/index.html and play with <example>)

Note: I've popped into the polymer slack if anyone wants more back-and-forth questions, and we've also got conversations going on about this in the Discord for https://package.community. I probably won't say much more in this thread, specifically, but I figured this'd be useful. ❤️

ChadKillingsworth commented 7 years ago

I'm all for discussions on this type of front, but in the short term the flat attribute still needs removed from any package designed to be a dependency. It's breaking yarn today.

TimvdLippe commented 6 years ago

@FredKSchott could you give an update about the status of yarn --flat? I found https://github.com/Polymer/polymer-modulizer/issues/254 but not sure what the latest status is.

justinfagnani commented 6 years ago

@TimvdLippe I think we'll have an update on this shortly.

markcellus commented 6 years ago

@justinfagnani any update on this? I've opened https://github.com/Polymer/tools/issues/403 where I'm proposing a different way for polymer-cli to resolve to a single package location that won't require use of yarn or a flat dep structure. Would love to get thoughts on that or at least know how the team is moving forward to remove this package's reliance on yarn's flat dep structure.

markcellus commented 6 years ago

fyi, not sure why commenting on this thread unassigned someone from it. Note: that happened automatically and by nothing specifically done on my part