jspm / generator

JSPM Import Map Generator
Apache License 2.0
160 stars 20 forks source link

Import map not capturing multiple versions of a library listed as a peer dependency in multiple packages #284

Closed zachsa closed 1 year ago

zachsa commented 1 year ago

After adding a library, and getting the following error on page load:

client.js:24 Uncaught TypeError: e.hydrateRoot is not a function
    at r.hydrateRoot (client.js:24:16)
    at m (main-b1c1d123.js:22:123)
    at index.forecast.js:55:1

I found that when two libraries specify different versions for peer dependencies, that only one peer dependency version is included in the import map https://ga.jspm.io/ scope.

@mui/material

"peerDependencies": {
  ...
  "react": "^17.0.0 || ^18.0.0",
  "react-dom": "^17.0.0 || ^18.0.0"
},

react-data-grid

"peerDependencies": {
  "react": "^16.14 || ^17.0",
  "react-dom": "^16.14 || ^17.0"
}

The import map

<script type="importmap">
  {
    "imports": {
      "@mui/material/Paper": "https://ga.jspm.io/npm:@mui/material@5.11.14/Paper/index.js",
      "react": "https://ga.jspm.io/npm:react@18.2.0/dev.index.js",
      "react-data-grid": "https://ga.jspm.io/npm:react-data-grid@7.0.0-canary.49/lib/bundle.js",
      "react-dom": "https://ga.jspm.io/npm:react-dom@18.2.0/dev.index.js",
      "react-dom/client": "https://ga.jspm.io/npm:react-dom@18.2.0/dev.client.js",
      ...
      "react/jsx-runtime": "https://ga.jspm.io/npm:react@18.2.0/dev.jsx-runtime.js"
    },
    "scopes": {
      "../": {
        ...
      },
      "https://ga.jspm.io/": {
        "react": "https://ga.jspm.io/npm:react@17.0.2/dev.index.js",
        "react-dom": "https://ga.jspm.io/npm:react-dom@17.0.2/dev.index.js",
        "react/jsx-runtime": "https://ga.jspm.io/npm:react@17.0.2/dev.jsx-runtime.js",
      },
      ...
      "https://ga.jspm.io/npm:react-dom@18.2.0/": {
        "scheduler": "https://ga.jspm.io/npm:scheduler@0.23.0/dev.index.js"
      }
    }
  }
</script>

I was expecting React 17 to be included in a separate scope - https://ga.jspm.io/npm:react-data-grid@7.0.0-canary.49/, or in general to be able to reference multiple versions of react in the import map if that is what the libraries specify peer dependencies. Is this expected?

If I manually change the react versions in the https://ga.jspm.io/ scope to v18, then everything works.

Please let me know if there is a way to overwrite peer dependency versions so that I can force the react imports in the ga.jspm.io scope to be v18.

zachsa commented 1 year ago

Looking at the react-data-grid GitHub repository I'm not sure why I got the @7.0.0-canary.49 version actually. In my package.json file the entry is:

"react-data-grid": "^7.0.0-beta.28",

Which specifies React peer dependency as v18. And fixing the entry to that version "react-data-grid": "7.0.0-beta.28", resolves my immediate problem.

Bubblyworld commented 1 year ago

Hey, yeah, right now the generator treats a peerDependency the same way as any other dependency, and just adds a constraint to the parent scope for it. We should be doing it properly, i.e. constraining the nearest parent scope with the peer dependency installed instead (and warning/throwing if that's violated).

It looks to me like your dependencies in the first example are inconsistent: react-data-grid@7.0.0-canary.49 is a plugin for react versions ^16.14 || ^17.0, but you've installed react@18.2.0, so that's likely why it errored. The peerDependency constraint for react-data-grid@7.0.0-beta.28 is ^18.0, so I think that's why changing it fixed the error.

If you want to force a particular version range for a package in all contexts, you could try using resolutions:

let generator = new Generator({
  mapUrl: import.meta.url,
  env: ["production", "browser"],
  resolutions: {
    react: "^18.0.0",
  },
});
guybedford commented 1 year ago

In the past the treatment I've used for peer dependencies has been to treat them as top-level dependencies as if they were primary ranges instead of secondary ranges. Then when new dependency ranges apply over that same package name to intersect the ranges or if there is no intersection to use the higher range, possibly with a warning when that conflict arises and a choice is made.

By treating peer dependencies as "discovered" primaries we achieve the design goal for peer dependencies that there is only one version of them in the graph between all consumers, while warning on conflicts.

That said the above design does conflict with having peer dependencies being allowed to branch at all, which might be useful in some specific cases eg when using resolutions. I'm not sure if npm allows this or not though.

In this specific case it does seem to me like React 17 is the best intersection of the peer dependency goals though. In the description of first design that seems to be operating correctly. Resolutions as the correction makes sense. So certainly not sure if there's even a bug here!

zachsa commented 1 year ago

Thank you for the responses @Bubblyworld and @guybedford.

In terms of treating peer dependencies as top level vs actual dependencies. I would have thought that most libraries bundle their dependencies into the bundle.js or equivalent. When I import a library that has bundled deps into it, I was just expecting to get multiple copies of those dependencies through the JSPM CDN. Is this correct?

I've never really understood the scopes part of import maps actually. In the import map (re-pasted below) I've got imports and scopes. My understanding is that direct import (i.e. if I have import @mui/material/Paper in the index.js file) are registered as imports. And that indirect imports are added as scopes. Is this correct?

In this import map I have 3 scopes:

  1. ../
  2. https://ga.jspm.io
  3. https://ga.jspm.io/npm:react-dom@18.2.0/

What do each of these mean?

Now that I'm looking at this... Is it correct to say that indirect imports are scoped to locations in the import map? If so why?

I would have expected the keys of the scopes object to be library versions. I.e. if react-data-grid imports react-dom, then I would have expected a lookup of scopes['react-data-grid']['react-dom'] which would resolve to react-dom@17.0.2 that would be a key in the imports object that I would have to ensure existed.

  {
    "imports": {
      "@mui/material/Paper": "https://ga.jspm.io/npm:@mui/material@5.11.14/Paper/index.js",
      "react": "https://ga.jspm.io/npm:react@18.2.0/dev.index.js",
      "react-data-grid": "https://ga.jspm.io/npm:react-data-grid@7.0.0-canary.49/lib/bundle.js",
      "react-dom": "https://ga.jspm.io/npm:react-dom@18.2.0/dev.index.js",
      "react-dom/client": "https://ga.jspm.io/npm:react-dom@18.2.0/dev.client.js",
      ...
      "react/jsx-runtime": "https://ga.jspm.io/npm:react@18.2.0/dev.jsx-runtime.js"
    },
    "scopes": {
      "../": {
        ...
      },
      "https://ga.jspm.io/": {
        "react": "https://ga.jspm.io/npm:react@17.0.2/dev.index.js",
        "react-dom": "https://ga.jspm.io/npm:react-dom@17.0.2/dev.index.js",
        "react/jsx-runtime": "https://ga.jspm.io/npm:react@17.0.2/dev.jsx-runtime.js",
      },
      ...
      "https://ga.jspm.io/npm:react-dom@18.2.0/": {
        "scheduler": "https://ga.jspm.io/npm:scheduler@0.23.0/dev.index.js"
      }
    }
  }
Bubblyworld commented 1 year ago

Hey, there's quite a bit to unpack:

I've never really understood the scopes part of import maps actually. In the import map (re-pasted below) I've got imports and scopes. My understanding is that direct import (i.e. if I have import @mui/material/Paper in the index.js file) are registered as imports. And that indirect imports are added as scopes. Is this correct?

There are two different things going on here. First, for import maps in general (not just the ones JSPM produces), "imports" are used to map bare specifiers like react to actual module URLs, and "scopes" are used to change those mappings in particular contexts. For instance, in your map above, you have an "imports" mapping for react to [...]react@18.2.0[...]. This means that if you were to call import("react") in a web page with this map installed, it would fetch that module from ga.jspm.io.

However, you also have a "scopes" mapping in https://ga.jspm.io/ for react to [...]npm:react@17.0.2[...]. This means that if any module in that scope (i.e. anything fetched from ga.jspm.io) imports the bare specifier "react", it'll get that version of react instead. Scopes apply to any module that is fetched from a subpath of the scope key, with more specific scopes taking precedence.

In theory you can organise imports and scopes however you like. JSPM takes the following approach:

  1. top level dependencies in modules that are traced or installed go in the "imports"
  2. secondary dependencies go in "scopes"
  3. scopes are flattened as much as possible, i.e. pushed down to the least-common-URL, similarly to how npm flattens the node_modules folder as much as possible. This is why most of the secondaries are under the https://ga.jspm.io scope.

In the case of ../ is this a relative path? If so, what it relative to and from?

Yes, it's relative to the URL of the HTML page that installs the import map. So if the import map was installed into https://foo.bar/my/page.html, the mappings in this scope will apply to all javascript modules under https://foo.bar/my/.

Why would my imports be scoped to https://ga.jspm.io/?

Because JSPM flattens scopes to save space in the import map whenever it can do so (i.e. when doing so doesn't violate a dependency constraint).

What could cause there to be a scope of https://ga.jspm.io/npm:react-dom@18.2.0/.

This is because the scope couldn't be flattened for some reason - probably because one of your other dependencies requires a different version of scheduler, and flattening the scope would have violated that constraint.

Does that make sense? There's a lot going on - you could try reading the import maps spec for some more details, it's not super technical.

zachsa commented 1 year ago

Thank you very much for the helpful response - I realize that I was essentially asking how import maps work, and that your answer went well beyond the scope of the JSPM generator. The spec may not be super technical, but it's still a lot to take in! It's a little easier to read now that I have some more context (as in, I'm actively working with import maps)

I'm pretty happy to finally understand how scoping is handled in the import maps! It's surprising to me that it has to do with the origin of a particular library rather than by library version.

Is it possible to have multiple module providers currently?

i.e. if I want to publish a package to consume via an import map that is configured to use JSPM as the provider. Do I have to publish that module to NPM so that it gets transpiled and republished via JSPM? What if I want that particular module to be made available via a different CDN?

Is this possible? One example that comes to mind is that ESRI publishes their JS library in ESM format via their own CDN: https://developers.arcgis.com/javascript/latest/es-modules/#using-the-esm-cdn.

I think there is a possibility that library providers (such as ESRI) stop publishing their JS via NPM so as to force people to use their own module CDNs. This could be mechanism to prevent public disclosure of their library usage metrics, but also so that they have access to usage stats.

... this is a bit on a tangent now, but the reason I thought of it was that, prior to my corrected understanding of import maps I would have assume that if an ESRI JavaScript module tries to import, for example, scheduler, the a single scheduler import mapping would be useable by both modules imported from ga.jspm.io and js.arcgis.com. But now I know that there would (in this theoretical case, if it's possible to support multiple providers in the same map) be two scope entries: ga.jspm.io and js.arcgis.com, and that the scheduler, when imported by libraries served from JSPM, would be fetched from JSPM. And when imported by libraries served via js.arcgis.com, scheduler would be served from their instead.

zachsa commented 1 year ago

Actually, thinking about this... I think it makes sense that import map resolution would not support CORS requests for imports from imported modules.

Would that have security implications?

Bubblyworld commented 1 year ago

Yeah, the generator is pretty geared towards the single-provider use case at the moment, although there is some support for dividing up package space into different providers, using the providers option. And you can register custom CDNs quite easily with custom providers. (we've been testing the CDN builder like this, for instance - build packages to the filesystem, write a custom provider against them, use jspm to cook up import maps and test the builds work)

I think there are some edge cases to work through still though 👀

The policy is something like this at the moment, for a mapping in the import map:

zachsa commented 1 year ago

Thanks - I hadn't seen that documentation before. It's very comprehensive. If I find some edge cases I'll be happy to post them!