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

flag for packages that need to be unique #1

Open
daKmoR opened this issue Oct 20, 2018 · 8 comments
Open

flag for packages that need to be unique #1

daKmoR opened this issue Oct 20, 2018 · 8 comments

Comments

@daKmoR
Copy link

daKmoR commented Oct 20, 2018

First thanks for starting something like this 👍

I am curius where you wana go with it e.g. what is planned? for example did you see the possible overhaul of the package-name-maps (maybe even with rename)? WICG/import-maps#53

I would like to use something like this to generate maps that can be used with web components.
However they need to be unique.

Given the following dependency tree

$ npm ls
├─┬ test-webcomponent-feature-a@0.0.2
│ └── test-webcomponent@1.0.0
└─┬ test-webcomponent-feature-b@0.0.3
  └── test-webcomponent@2.0.0

We should get an Error/Question while creating package name map.

Possible example flow:

Could not find a sadisfying version for 'test-webcomponent'. Which one would you like to use in your package name map?
  > v1.0.0 (required by test-webcomponent-feature-a@0.0.2)
    v2.0.0 (required by test-webcomponent-feature-b@0.0.3)

I made an example Repository
https://github.com/daKmoR/build-pnm-webcomponent-issue

and published all needed packages to test it.

I'm curious on what you think? if I can help out in any way just let me know.

@billiegoose
Copy link

billiegoose commented Oct 20, 2018

I think this is outside the scope of all current bundlers / module installers. For instance, npm and yarn (or webpack) have no such protection mechanisms do they?

JavaScript registers web components using an imperative API (customElements.define) and the "name" field in package.json has no bearing on what element[s] that code might register.

@daKmoR
Copy link
Author

daKmoR commented Oct 20, 2018

copied over from issue on package-name-maps:

@arcanis Actually, it's not a requirement of pnm - but first recap the main goal of pnm as far as I understood.
pnm should enable us to use bare modules within the browser without a build step
e.g. you can upload your full directory/source code as is and put it on a plain web/file server and it should work.

Keeping that in mind it means that we may have a lot of files on our server but we still want to ship as little a possible to our users/browser.

Naturally, that means we don't want to ship code doing the same thing multiple times. So using like 4 versions of for example camelcase in our application may be a bad idea regarding the file size - but it should still work. That is the reason why pnm supports multiple versions of a package. I would still find a warning, in that case, would be appropriate [1].

Entering Webcomponents

For custom elements it's just not possibele to use different versions of the same element. It's a requirement of CustomElementRegistry. The relevant part is

If this CustomElementRegistry contains an entry with name name, then throw a "NotSupportedError" DOMException.

In good old english this means: "Each custom elements tag name needs to be unique within a browsers Window object".
Actually from a browsers point of view it's obvious there can only be one <video> tag.

Also a nice comment about it is WICG/import-maps#5 (comment).

PS: there have been some workaround like creating unique custom element names by adding random strings but they definitely violate the idea to only ship as little as possible.


[1]: This could be more than just a warning. You could even give some info on how to pin to certain versions that should be used. e.g. something like this,

The package "camelcase" is used with the following versions 1.2.2, 1.3.0, 2.0.0, 2.0.9. You can run "pnm set-resolution camelcase 1.3.3 2.1.0" to only use the latest releases of each major version.

PS: Have a safe flight 👍

@daKmoR
Copy link
Author

daKmoR commented Oct 20, 2018

@wmhilton: neither yarn nor npm have a way to override or limit the amount of versions that you use. In webpack you can do use resolutions to archive it and/or use tools like duplicate-package-checker-webpack-plugin to get a warning.

So, in short, you can solve it with build tools but not with packager tools. There are some movement in this direction but no working implementation yet[1] and changes to npm and yarn always take quite some time and usually have a huge impact (as so many people use it)

So having a specialized generator for package name maps that support a unique flag wound a preferred solution I would say. And it would definitely be awesome.

JavaScript registers web components using an imperative API (customElements.define) and the "name" field in package.json has no bearing on what element[s] that code might register.

That is true but we are not interested in the name (as on package can have multiple custom elements) we just want to make sure that certain packages are only ONCE in the package name map.

e.g.

import 'fireCollection/my-foo.js'; // customElements.define('my-foo', ...)
import 'fireCollection/my-bar.js'; // customElements.define('my-bar', ...)

is totally fine as long as fireCollection is only ONCE available. As es modules will never load a same file twice... e.g. if you later do import 'fireCollection/my-foo.js'; nothing will happen.

However if those are different versions then they resists in different locations so it will be loaded again... and boom DOMException

[1]: if you exclude yarn --flat as it will mingle up your devDependencies which you want deep (only your "browser" dependencies you want flat)

@arcanis
Copy link
Owner

arcanis commented Oct 20, 2018

I am curius where you wana go with it e.g. what is planned? for example did you see the possible overhaul of the package-name-maps (maybe even with rename)? WICG/import-maps#53

So this project started as a proof-of-concept. I wanted to show that it was possible (and easy) to generate any type of data structure simply by extracting the data from the PnP API. Since the pnm proposal is something I kept an eye on, it was a good candidate 😃

Overall, I think it's important to stay close to the layout provided by the package manager. As you mentioned, the goal of pnm (if I understood correctly) is to be able to use the dependency as-is, by serving the installed app. To make it truly isomorphic, it has to use the same dependency tree than the one used by Node.

Naturally, that means we don't want to ship code doing the same thing multiple times. So using like 4 versions of for example camelcase in our application may be a bad idea regarding the file size

While I understand the size concern, I'm concerned about the burden of manually solving conflicts. That could in my mind be a big barrier to the pnm adoption. We would have to check in real case scenarios, but I suspect a lot of dependency ranges would conflict, requiring users to manually solve the conflicts (which is quite hard since they usually have no idea which one to use) 🤔

There are some movement in this direction but no working implementation yet

I think I quite like what @ljharb suggested - to enforce the "flatness" by using peer dependencies. There are some pitfalls that have to be considered, though:

  • In its current state, it will break the "yarn add foo and it works" workflow, since the user would also have to manually install the peer dependencies for foo (it's already the case, but peer dependencies are relatively rare so that's fine). Maybe we could add a flag to tell that a peer dependency should be auto-added, but it seems a bit too specific to me.

  • The current PnP spec guarantees that a package with peer dependencies will be instanciated once for each time it is found in the tree. It's not a problem if it is a peer-dependency itself (because then it would only appear once in the dependency tree), but that makes it quite easy to get weird errors that might be hard to debug. Some tooling would be welcomed to ensure that we can catch those errors at install-time.

  • It only works if the ecosystem uses this pattern (meaning a lot of teaching, without guarantee that it will propagate enough), and it's a technical solution, not a semantical one (meaning that we won't be able to swap the implementation for a better one later on).

PS: Have a safe flight 👍

Thanks @daKmoR! 😃

@daKmoR
Copy link
Author

daKmoR commented Oct 20, 2018

I may have not explained well enough what I would expect 🙈

I definitely don't want want to have a global flag so that users will need to choose for every package. But for packages that are "naturally" unique like web components it would help to catch the error while installing and not while opening the browser. Sending them both to the browser or even have a mapping for it is not usefull at all as you just can't load it.

I would like to quote @zkat here

The Node.js world, by contrast, has been much more aggressive about small-modules, and thus a potential increase in version conflicts, because the Node algorithm has always protected them from that.

for webcomponents in the browser directly there is NO protection we can offer - it's really awesome that you don't need to care about it in node (or even react?)... but if you use webcomponents the browser (rightfully so) forces you.

I'm sorry for beeing so persitent but it seem I may be a dreamer that still believes in a world where you don't need any build tools while developing. (I do not consider yarn install a build step)

Let my try slightly different. Let's extend my demo repository with another dependency

// package.json
  "dependencies": {
    "test-webcomponent-feature-a": "^0.0.2",
    "test-webcomponent-feature-b": "^0.0.3",
    "webpack": "^4.0.0" 
  },

something like this should give me the question "Could not find a sadisfying version for 'test-webcomponent ..." but ONLY because I have configured "test-webcomponent" to be unique. webpack will be installed nested if needed and nothing will change for it.

possible configuration for unique packages could be

// package.json
{
  "name": "test-webcomponent",
  "version": "2.0.0",
  "devDependencies": {
    "http-server": "^0.11.1"
  },
  "build-pnm": {
      "unique": true
   }
}

to summarize:

  • ONLY "marked as unique" packages will raise questions if available in multiple versions
  • having them available in multiple versions would not allow them to be loaded
  • webcomponent registry is global (you can't scope it)
  • this could be archived just by generating a package-name-map (no change in node or yarn needed)

@billiegoose
Copy link

billiegoose commented Oct 24, 2018

TLDR; @daKmoR I completely disagree with your reasoning but agree that the requested feature is a good one.

The disagreeing part:

I still think you're conflating WebComponents with modules.

Let's say I have a module that exports a webcomponent, MyFoo.

You could have multiple versions of that module in the graph. You just need to make sure that only one copy of the module actually registers MyFoo. The best modules export pure functions, and don't run any code when they are imported, and certainly don't register globals. You don't need to bring WebComponents into the conversation to see this - all we need to do is think back to ye old jQuery days. We don't want multiple versions of jQuery attempting to attach themselves to the window object.

It's pretty straightforward to say, "well, if you're publishing WebComponents that auto-register on import, you're doing it wrong" and toss the problem to userspace.

BUT.

That's not very helpful. 🤷‍♂️

The agreeing part:

@daKmoR brought up the duplicate-package-checker-webpack-plugin. I use that plugin extensively in my own work to prevent bundle bloat. And I do think that it would be awesome to have a way to force certain packages to use certain versions. That would let application developers solve/workaround the jQuery / WebComponent problem, regardless of whether module authors wrote misbehaving modules that register globals. Webpack in particular has a lot of flexibility to hack "workarounds" around existing packages that don't play well with the web. So tools that generate package name maps for the web should also (ideally) provide a similar level of hackability.

However, it seems very likely that the root of the issue is not in build-pnm at all, but ought to be addressed in pnm itself. If build-pnm just uses the pnm API, then adding custom module resolution aliasing and rewriting like we're describing would be a kind of tacky addon to this tool, imho. Since pnm uses code to define it's module resolution, I feel like this could be addressed in a future upgrade to the pnm library.

@arcanis
Copy link
Owner

arcanis commented Oct 24, 2018

You just need to make sure that only one copy of the module actually registers MyFoo. The best modules export pure functions, and don't run any code when they are imported, and certainly don't register globals.

Yes and no. Imagine the following: you write a component Foo. Foo@1 accepts a prop named cb, and Foo@2 renames it to callback. Your application depends on Bar (which depends on Foo@1) and Baz (which depends on Foo@2).

Even if Foo doesn't register itself automatically (let's say it exposes a setup function that, when called, calls window.customElements.define('Foo', ...)), you still can't call the setup from both Foo@1 and Foo@2 - meaning that one of Bar and Baz will break, depending on which version of Foo got its setup function called. So it's not only a problem of impure packages, but rather the sheer fact that some packages are designed to be singletons, for the better or the worse.


I think more generally the problem of singleton packages is something that should be solved by the package managers. It's far from being a Javascript-only problems: taking C and C++ as example, you can't link identical symbols together (unless static storage). This is the same here.

The main question I'm still thinking about is whether it should come from the user or from the package. Should the user specify that package Foo should be singleton'd? Or should the package Foo says that it must be singleton'd (similar in some way to the flat: true settings we currently have on Yarn)?

@daKmoR
Copy link
Author

daKmoR commented Oct 24, 2018

but rather the sheer fact that some packages are designed to be singletons, for the better or the worse.

yes that is the main point 🤗

very simple example

// v1.0.0/foo.js
window.fooFlag = 'BAR';

// v2.0.0/foo.js
window.fooFlag = 'BAZ';

// app.js
import 'v1.0.0/foo.js';

if (window.fooFLAG === 'BAR') { ... } // e.g. depending on v1

so if ANY package you use is now loading the v2 version... this will just break... problem is it will silently break and stuff like that is usually tough to find and fix.

I think more generally the problem of singleton packages is something that should be solved by the package managers

I totally agree that it should be solved on the npm or yarn side... but to be honest I have given up on that hope 🙈 (at least on the hope that it happens any time soon).
So I hoped for the package name maps - but maybe you are right and it's the wrong place to do it by default... writing something on top of that to give warnings or errors for double versions like the plugin for webpack should be easily double.

The main question I'm still thinking about is whether it should come from the user or from the package.

I would go with that the package author sets it... he knows if the package code needs to be unique or not.
Checking for every dependency that I have if it should be unique or not would be tough - especially for transitive dependencies.

so if I do npm i foo it could be that it installs 3 packages "deep" and 2 packages as singleton... and foo itself could allow for "deep" installs or not (e.g is itself a singleton or not)

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

3 participants