jspm / project

Roadmap and management repo for the jspm project
159 stars 8 forks source link

jspm.dev import forks sub-dependency, imports two versions #291

Closed trusktr closed 6 months ago

trusktr commented 9 months ago

Example:

<script defer>
  const start = performance.now()
</script>

<script type="module">
  import {defineElements} from 'https://jspm.dev/lume@0.3.0-alpha.27'

  console.log('time to load:', performance.now() - start)

  defineElements()
</script>

playground link

Output:

You appear to have multiple instances of Solid. This can lead to unexpected behavior.

Inspecting Sources tab shows that Solid 1.4, and 1.7, are both imported.


However, the import map generator does not seem to have the issue for lume:

https://generator.jspm.io/#U2VhYGBkDM0rySzJSU1hyCnNTXUw0DPWM9BNzCnISNQzMgcAMJcFxyIA

Example without duplicate import:

<!--
  JSPM Generator Import Map
  Edit URL: https://generator.jspm.io/#U2VhYGBkDM0rySzJSU1hyCnNTXUw0DPWM9BNzCnISNQzMgcAMJcFxyIA
-->
<script type="importmap">
  {
    "imports": {
      "lume": "https://ga.jspm.io/npm:lume@0.3.0-alpha.27/dist/index.js"
    },
    "scopes": {
      "https://ga.jspm.io/": {
        "@lume/autolayout/es/AutoLayout.js": "https://ga.jspm.io/npm:@lume/autolayout@0.9.1/es/AutoLayout.js",
        "@lume/custom-attributes/dist/index.js": "https://ga.jspm.io/npm:@lume/custom-attributes@0.1.7/dist/index.js",
        "@lume/element": "https://ga.jspm.io/npm:@lume/element@0.7.1/dist/index.js",
        "@lume/eventful": "https://ga.jspm.io/npm:@lume/eventful@0.2.5/dist/index.js",
        "@lume/kiwi": "https://ga.jspm.io/npm:@lume/kiwi@0.3.4/es/kiwi.js",
        "@lume/three-projected-material/dist/ProjectedMaterial.js": "https://ga.jspm.io/npm:@lume/three-projected-material@0.2.5/dist/ProjectedMaterial.js",
        "@lume/variable": "https://ga.jspm.io/npm:@lume/variable@0.8.0/dist/index.js",
        "element-behaviors": "https://ga.jspm.io/npm:element-behaviors@3.1.0/dist/index.js",
        "james-bond": "https://ga.jspm.io/npm:james-bond@0.5.1/dist/index.js",
        "lowclass": "https://ga.jspm.io/npm:lowclass@5.0.1/dist/index.js",
        "regexr": "https://ga.jspm.io/npm:regexr@1.6.0/index.js",
        "solid-js": "https://ga.jspm.io/npm:solid-js@1.4.8/dist/dev.js",
        "solid-js/html": "https://ga.jspm.io/npm:solid-js@1.4.8/html/dist/html.js",
        "solid-js/store": "https://ga.jspm.io/npm:solid-js@1.4.8/store/dist/dev.js",
        "solid-js/web": "https://ga.jspm.io/npm:solid-js@1.4.8/web/dist/dev.js",
        "three": "https://ga.jspm.io/npm:three@0.157.0/build/three.module.js",
        "three/": "https://ga.jspm.io/npm:three@0.157.0/"
      },
      "https://ga.jspm.io/npm:@lume/variable@0.8.0/": {
        "lowclass": "https://ga.jspm.io/npm:lowclass@4.9.6/dist/index.js"
      }
    }
  }
</script>

<script defer>
  const start = performance.now()
</script>

<script type="module">
  import {defineElements} from 'lume'

  console.log('time to load:', performance.now() - start)

  defineElements()
</script>

playground link

JayaKrishnaNamburu commented 9 months ago

@trusktr this is a expected behaviour. And one of the main reason behind the support for import-maps. Here is a dependency visualisation of the lume package https://npm.anvaka.com/#/view/2d/lume/0.3.0-alpha.27

Screenshot 2023-10-09 at 12 53 35

Turns out, the sub-deps are specifying different versions of the solid-js under them. And the jspm.dev which works without the import-maps always tries to resolve the dependencies with it's own context. Whereas when you are using import-maps, the resolution takes into consideration of the requirements of the rest of the packages too. And so, the issue doesn't pop-up when you are using import-maps

trusktr commented 9 months ago

@JayaKrishnaNamburu I think that this behavior is very unexpected for anyone in the JS ecosystem. Here's why.

The vast majority of the JavaScript world is using Node.js and NPM's module resolution strategy, which includes deduping packages with compatible version ranges. JSPM.dev is not doing this (unlike the importmap generator, which is inconsistent).

If you take a look at lume's package.json for 0.3.0-alpha.27, you'll see that solid-js is specified with the range >=1.0.0 <1.5.0.

Now, if you look at the package.json of one of the other packages that depend on solid-js in that graph, f.e. @lume/element's package.json, you'll see that solid-js is specified with the range ^1.0.0.

Based on both of these ranges, >=1.0.0 <1.5.0 and ^1.0.0, all major NPM package managers will resolve this to a single version that satisfies both ranges (deduped), and an application with dependencies installed using an NPM package manager will work without issues.

That's the whole point of specifying version ranges.

So, because jspm.dev is not applying version range rules like everyone is accustomed to, this will lead to people's code not working in an unexpected way, and defeats the purpose of having version ranges.

This is why I believe this should be accepted as a valid bug.

Making unnecessary semver-compatible dependeny forks is a recipe for errors.


Maybe the issue is jspm.dev is not implemented to take that into account (which is bad!), and it should be updated.

guybedford commented 9 months ago

@trusktr jspm.dev is called jspm.dev because it is for development and experimentation and never for production workflows, exactly because it has this limitation. Working out how to communicate that is the hard part :)

guybedford commented 9 months ago

I personally never use jspm.dev and wouldn't recommend it to anyone. jspm.io is the CDN.

trusktr commented 9 months ago

jspm.dev is called jspm.dev because it is for development and experimentation and never for production workflows, exactly because it has this limitation. Working out how to communicate that is the hard part :)

I personally never use jspm.dev and wouldn't recommend it to anyone. jspm.io is the CDN.

Maybe just mention the issue here?

https://jspm.org/cdn/jspm-dev

(but that's a fairly critical issue, I do not believe that even if it is for development that it should be making dependency forks)

Another way to phrase that is with this question: how can I possibly use that for development if it breaks my code (because it doesn't follow basic conventions of other tools like yarn, npm, pnpm)?

Even if it is for development, forking dependencies makes it impossible to use for development when singletons are needed.

We're talking about two separate issues:

guybedford commented 8 months ago

So jspm.dev does 100m requests per month for 320GB transfer. This is against jspm.io which does 1.5 billion requests per month for 10.2TB of transfer. In addition while jspm.io traffic grows by around 10% each month, jspm.dev traffic is only growing about 2% each month.

Perhaps the path forward here is actually to just deprecate jspm.dev with a 2 year LTS shutting it down. This would save around $700 a month which would be nice.

The only reason I can think of for keeping jspm.dev around, is basically for when I'm in a browser console and just want to import a library quickly.

trusktr commented 8 months ago

import('https://jspm.dev/lume@0.3.0-alpha.30') is still causing dependency forks of solid-js, but the version ranges are definitely compatible, while Generator can see the compatible ranges and does not make a version fork.

Perhaps the path forward here is actually to just deprecate jspm.dev with a 2 year LTS shutting it down

Yeah, I think fixing this issue, or deprecating jspm.dev, are the two only viable paths. Keeping it as is is very problematic.


Being able to import without import maps is super useful, especially in cases where we don't control the HTML payload (for example browser extensions that inject code into the page after the fact).

If you shut down jspm.dev, will jspm.io get that feature? Maybe it can be behind a flag like https://ga.jspm.io/npm:lume@0.3.0-alpha.30/dist/index.js?bare-specifiers=npm

guybedford commented 6 months ago

jspm.dev has now been fully deprecated, so closing.

guybedford commented 6 months ago

Blog post - https://jspm.org/jspm-dev-deprecation