skypackjs / skypack-cdn

An issue tracker for the CDN
107 stars 5 forks source link

React version locked at 17.x #88

Open pvormittag opened 3 years ago

pvormittag commented 3 years ago

My team noticed one of our applications failing today and the issue is that Skypack is returning the latest version of React and not respecting the version designation on the URI (e.g. @^16.13.1).

I've attached screen shots showing the request from both a browser (Chrome) and cURL.

Screen Shot 2020-12-09 at 1 44 28 PM Screen Shot 2020-12-09 at 1 45 34 PM
drwpow commented 3 years ago

Sorry to hear you’re experiencing this issue. As you’ve discovered, React is getting a more recent version than the semver requested. That’s somewhat intentional, but I think it’s unexpected that you’re experiencing issues.

We have to treat React differently from other packages on Skypack due to the infamous multiple versions issue. If a different version of React tries to re-render an already mounted component created by another version, React will throw an error. React 17 fixed this (or at least, is trying to).

If you look through your dependency tree of your app and all the components your React deps are loading, you’ll probably see many possible versions of React trying to load. But when you run npm install, only 1 version of React will end up being saved. That is your version your app will use, and the multiple versions problem probably isn’t an issue when you were bundling & deploying a local version yourself.

But with Skypack, everything is loaded on-demand. So for all your dependencies, each trying to load their own version of React (most of which is ^16.x.x), we were seeing a lot of “multiple copies of React loaded“ errors. To fix it, React is the only dependency that we version-clamp behind-the-scenes. We went with React 17 because we believed it to be backward compatible with 16.x. For the most part, we’ve seen an improvement of the React loading story for most people; this is the first time someone’s reported having an issue.

So all that said, I’d love to learn more about what your dependency tree look like, e.g.:

Unfortunately it’s difficult to simply undo the React locking we have in Skypack because of the reasons mentioned. But I’d love to know if there were some solution that we could figure out for you.

pvormittag commented 3 years ago

Thanks for the detailed explanation @drwpow!

The application in this case is a simple single page form. We are able to rely completely on modern browsers, so we took the opportunity to develop the application using only ES modules and without the need for the normal, overly complex toolchain the JS ecosystem has come to have over the years. Therefore the application does not rely on Node or NPM. It's simply a main.js module as the entrypoint with import statements directly from Skypack for all module dependancies and resolution.

Examples from the application code:

import React from 'https://cdn.skypack.dev/react@^17.0.1';
import { render } from 'https://cdn.skypack.dev/react-dom@^17.0.1';
import html from './html.js';
import ReportForm from './components/ReportForm.js';
import React, { useState, useEffect } from 'https://cdn.skypack.dev/react@^17.0.1';
import html from '../html.js';
import { Formik, Field, useFormikContext } from 'https://cdn.skypack.dev/formik@^2.1.5';
import * as Yup from 'https://cdn.skypack.dev/yup@^0.29.1';

We ended up addressing the issue by simply upgrading the dependency that wasn't handling the newer version of React. In this case it was SWR. We were on ^0.1.16 and upgrading to ^0.3.9 resolved the error.

It would be helpful if Skypack could somehow note these edge cases, otherwise directly having your dependencies bound to a CDN such as Skypack without the SemVer guarantee or a pinned version URL – which I couldn't determine in this case for the needed version of React – is risky business.

drwpow commented 3 years ago

Wow that’s awesome you’re able to shed so much complexity and build tools! Absolutely love to hear it, and that’s why we made Skypack ❤️

Also glad that you were able to fix the issue you had.

It would be helpful if Skypack could somehow note these edge cases, otherwise directly having your dependencies bound to a CDN such as Skypack without the SemVer guarantee or a pinned version URL – which I couldn't determine in this case for the needed version of React – is risky business.

Heard you loud and clear. Although React is the only dependency we do this for, you’re right—we absolutely should do a better job either at issuing a warning, or making it more transparent what’s happening behind-the-scenes. We are hoping that this is a temporary workaround, and once the React community is more transitioned into v17 and later versions, the problem fades. But in the mean time you’re absolutely right we should make this one edge case more clear, because from your end the problem/suggested solution isn’t currently clear.

pvormittag commented 3 years ago

@drwpow Thank you for taking in the feedback and keep up the great work! My team has enjoyed the experience with Skypack – and Pika prior – and believe in what Skypack is attempting to solve! ✌️

huw commented 3 years ago

Hey @drwpow—slightly related issue. I'm trying to use react@experimental to use Concurrent Mode, but because of the version locking, I'm unintentionally getting React 17. What's the best way to get the version of React I want in Skypack?

pvormittag commented 3 years ago

This has cropped up again for us as well in the same application.

The issue is still with SWR, which makes some sense given the package is expecting React 16.11.0.

The React error being thrown: https://reactjs.org/docs/error-decoder.html/?invariant=321

The import statement from the script were the error is propagating from:

import React from 'https://cdn.skypack.dev/react@^17.0.1';
import { Field, FieldArray, useFormikContext } from 'https://cdn.skypack.dev/formik@^2.1.5';
import useSWR from 'https://cdn.skypack.dev/swr@^0.3.9';

None of our code has been changed or redeployed, so I'm unsure why my previous solution of simply updating the dep version has broken now. I can only assume we are receiving different code now. I'm going to try to lock to exact version numbers for SWR and see if that at least reverts the regression in our case.


[UPDATE]: Locking to the exact version of 0.3.10 for SWR did revert the regression.

FredKSchott commented 3 years ago

Sorry that you all are hitting this, hopefully these packages upgrade to support React v17 soon!

pvormittag commented 3 years ago

@FredKSchott Thank you, and I understand how this isn't something Skypack can easily solve.

A suggestion for future readers: If you are manually taking module deps directly from Skypack as we are, then only rely on exact version numbers – or Pinned URLs – and avoid SemVer ranges. Given this approach does not come with support of a lock file concept today.

percyhanna commented 3 years ago

I'm confused why Skypack is also ignoring pinned versions, such as https://cdn.skypack.dev/react@16.8.4. This is returning 17.0.1 instead of the requested 16.8.4. I could somewhat understand the exception to return v17 for unpinned requests, but I'm not sure I understand the reasoning for not respecting pinned versions.

percyhanna commented 3 years ago

To follow up with more detail, we are using an import with a pinned version of React, but it is causing incompatibilities because some other modules are using an older version of React. This causes errors because hooks are failing with the mix of React versions. It seems unexpected and undesirable to swap out versions of a module for a pinned URL. Please roll back this behavior for pinned versions.

gingur commented 3 years ago

I understand the sentiment of pinning all requests to react@17, however I'm wondering if it would be possible to provide an opt-out of this feature? At a minimum, you should update your documentation to reflect this new odd behavior https://docs.skypack.dev/skypack-cdn/api-reference/lookup-urls#lookup-by-package-version-range-semver

drwpow commented 3 years ago

I'm confused why Skypack is also ignoring pinned versions

@percyhanna please see the above comment. The behavior applies to all URLs regarding React, because of runtime restrictions created by React.

drwpow commented 3 years ago

I understand the sentiment of pinning all requests to react@17, however I'm wondering if it would be possible to provide an opt-out of this feature? At a minimum, you should update your documentation to reflect this new odd behavior

It’s definitely something we’d consider for sure. We do want people to be able to be in control. We only implemented React version-locking because so many people reported errors from all their dependencies trying to load different versions. But for the people that know that their app and all dependencies can work on a specific React version, we should let you specify.

And yes—we do need to update our docs as well! Will do that.

roelandxyz commented 3 years ago

React 16 works in Deno, but React 17 doesn't yet: https://github.com/denoland/deno/issues/8440 JSX looks the same, but the resulting javascript is different: https://reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html

This should just return version 16 and not 17: import React from "https://cdn.skypack.dev/react@16.14.0";

shshaw commented 2 years ago

Now that React 18 is out, we have users at CodePen wanting to use that. Are there any ways around this version pinning so that users can specify which versions of React they want?

matthewp commented 2 years ago

Hey there! We talked about this and think we can remove the pinning now. Will spin it up and see if it doesn't break anything.

NyanHelsing commented 2 years ago

wouldn't users be able to work around the version issue here by using an import map and specifying/overriding the version of react to use for each scope?

NyanHelsing commented 2 years ago

wouldn't users be able to work around the version issue here by using an import map and specifying/overriding the version of react to use for each scope?

this seems to work...

    { "imports": {
        "react": "https://esm.sh/react@18.0.0",
        "https://cdn.skypack.dev/-/react@v17.0.1-yH0aYV1FOvoIPeKBbHxg/dist=es2019,mode=imports/optimized/react.js": "https://esm.sh/react@18.0.0",
        "https://cdn.skypack.dev/-/react@v17.0.1-yH0aYV1FOvoIPeKBbHxg/dist=es2019,mode=types/index.d.ts": "http://cdn.esm.sh/v76/@types/react@17.0.43/index.d.ts",
        "react-dom": "https://cdn.skypack.dev/react-dom@17.0.2?dts",
        "react-dom/server": "https://cdn.skypack.dev/react-dom@17.0.2/server?dts",
    }}
NyanHelsing commented 2 years ago

any word on this? its kind of a huge pain maintaining an import map to work around this

shshaw commented 2 years ago

We do have CodePen users trying to get React 18 from Skypack and ending up rather confused. Any updates would be appreciated :-)

marklundin commented 2 years ago

Will this be updated to 18.2.0 soon?

trhinehart-godaddy commented 2 years ago

Hey there! We talked about this and think we can remove the pinning now. Will spin it up and see if it doesn't break anything.

Any update on this @matthewp?

marklundin commented 2 years ago

FWIW @trhinehart-godaddy I went with esm.sh which resolves the correct React package

itismoej commented 2 years ago

Any updates here about supporting react 18? @drwpow

gaearon commented 2 years ago

We're using this for "Download" button in sandboxes on the React beta site, but it appears that it doesn't work with 18, so looks like we'll need to switch to something else.

zetaraku commented 2 years ago

Still wondering why the version is stuck at 17.0.1. 😕

Tracerract commented 2 years ago

We do have CodePen users trying to get React 18 from Skypack and ending up rather confused. Any updates would be appreciated :-)

I just spent my afternoon trying to figure out why my CodePen wasn't working because of this, so it was pretty validating to see your comments here @shshaw.

I went with esm.sh which resolves the correct React package

In case it can help anyone else, I ended up using this as well to get my CodePen working

veryjos commented 1 year ago

Ping, same issue.

NyanHelsing commented 1 year ago

this has been ages now with no change?

Im not using skypack anymore; they should take their site down and save money on hosting if they don't intend to maintain it. or advertise that they're looking for people to adopt the project.

I'm forced to just using esm.sh now.

NyanHelsing commented 1 year ago

@matthewp @drwpow Yall its been like over 2 years its time to remove this pinning. If it breaks something it breaks something; right now its doing more harm than good so pull the bandaid off.

jcubic commented 1 year ago

Any reason not to update React and have it hardcoded with version 17? This makes skypack useless.

Even using https://cdn.skypack.dev/react@18 make no difference and serve really old version of React.