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

Upgrade react version #1135

Merged
merged 7 commits into from
Oct 9, 2017
Merged

Upgrade react version #1135

merged 7 commits into from
Oct 9, 2017

Conversation

@luisrudge luisrudge added this to the v10-Next milestone Oct 6, 2017
Copy link
Contributor

@aaguiarz aaguiarz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should clarify in the Readme that tests require React16?

@luisrudge
Copy link
Contributor Author

Not sure this matters to anyone consuming the library.

@luisrudge luisrudge mentioned this pull request Oct 7, 2017
1 task
@luisrudge luisrudge merged commit ba43d86 into master Oct 9, 2017
@luisrudge luisrudge deleted the feature-upgrade-react branch October 9, 2017 14:01
@jglover
Copy link

jglover commented Oct 12, 2017

Any chance of a minor release with this fix soon?

@luisrudge
Copy link
Contributor Author

@jglover actually, if you follow here, we're rolling back this change to support only ^15.6.2 (we let the react@16 fixes in place, we're only rolling back the dependency). Because our dependency was too permissive, our customers are being upgraded to react@16 unintentionally. We'll fix that with the new release. We'll upgrade to react@16 in a future version (probably major).

@jglover
Copy link

jglover commented Oct 12, 2017

Thanks @luisrudge, that's understandable.
No chance of an auth0-lock-react16 npm package based off a branch with just those dependencies upgraded in the meantime? There are some useful features in React 16 that are a shame to miss out on.

@luisrudge
Copy link
Contributor Author

@jglover we're trying to figure it out how to help react@16 users. The ideal would be to move react and react-dom to peerDependencies, but this would be a breaking change for most of our customers, so we're thinking this through.

In the meantime, react@16 will work. you'll just have to bundle both versions if you're not using yarn. If you are using yarn, however, you can use the resolutions field and pin react@16 to your repo.

{
  "name": "test-test",
  "version": "0.1.0",
  "private": true,
  "dependencies": {
    "auth0-lock": "^10.22.0",
    "react": "^16.0.0",
    "react-dom": "^16.0.0",
    "react-scripts": "^1.0.14"
  },
  "scripts": {
    "start": "react-scripts start",
    "build": "react-scripts build",
    "test": "react-scripts test --env=jsdom",
    "eject": "react-scripts eject"
  },
  "resolutions": {
    "react": "16.0.0",
    "react-dom": "16.0.0"
  }
}

@scamden
Copy link

scamden commented Dec 8, 2017

@luisrudge fyi, i get two copies of react if i bundle auth0 into a component meant to be consumed from another component. my only current solution is to make auth0-lock a peer dep and mark it external to webpack

@spunkedy
Copy link

Is this going to get fixed for v11 at all?

@luisrudge
Copy link
Contributor Author

@scamden if you set the resolutions field (if you're using yarn), it will bundle only one copy.

@luisrudge
Copy link
Contributor Author

@spunkedy We'll probably only fix this for good when we release a new major version making react and react-dom peer dependencies. For now, we have a workaround for yarn users.

@spunkedy
Copy link

@luisrudge I looked at your merge, and the tags for the release. It doesn't have the || @16

I am using yarn and tried the resolution, doesn't seem to work with 11.3.0

@luisrudge
Copy link
Contributor Author

why it doesn't work? I have a sample here that works: https://github.com/luisrudge/lock-react16

@spunkedy
Copy link

@luisrudge https://gist.github.com/spunkedy/e8db600d8dfbec522070ceccbeeb379b

My resolutions are the same, my react is the same. I might be using it wrong, but I put a bunch of data in the gist.

The part that is concerning is that the lock file still shows:

auth0-lock@^11.2.3:
  version "11.3.0"
  resolved "https://registry.yarnpkg.com/auth0-lock/-/auth0-lock-11.3.0.tgz#8db0758126313f907d74a6930b14f65ff66f6914"
  dependencies:
    auth0-js "^9.3.0"
    blueimp-md5 "2.3.1"
    fbjs "^0.3.1"
    idtoken-verifier "^1.0.1"
    immutable "^3.7.3"
    jsonp "^0.2.0"
    password-sheriff "^1.1.0"
    prop-types "^15.6.0"
    react "^15.6.2"
    react-dom "^15.6.2"
    react-transition-group "^2.2.1"
    trim "0.0.1"
    url-join "^1.1.0"

any ideas?

@scamden
Copy link

scamden commented Feb 27, 2018

We don’t use yarn. I assume you’re going to support this for npm as well? We’re on npm 5

@luisrudge
Copy link
Contributor Author

@spunkedy that's fine. I get that in mine too: https://github.com/luisrudge/lock-react16/blob/master/yarn.lock#L301 What it matters is that the react version used will be whatever you set in your resolutions property.

@scamden Sadly, the workaround only works for yarn. npm has nothing like the resolutions field :(

@scamden
Copy link

scamden commented Feb 27, 2018

Ya makes sense. I meant in terms of moving react to a peer dep

@spunkedy
Copy link

@luisrudge

Then, any idea what might be going on?

const REACT_VERSION = React.version;
console.log(REACT_VERSION);

returns
16.2.0

@luisrudge
Copy link
Contributor Author

@spunkedy that's what you want, no?

@spunkedy
Copy link

haha yes, but without the auth0 lock errors :)

@luisrudge
Copy link
Contributor Author

@spunkedy what error? you didn't mentioned any

@luisrudge
Copy link
Contributor Author

@scamden we'll do that, but we need to do this in a new major release, because it will be a breaking change. This also means we'll have to update all docs, quick starts, samples etc. I assure you this will happen, but there's no ETA

@spunkedy
Copy link

@luisrudge
Copy link
Contributor Author

@spunkedy It's a compatibility issue with other packages you're using. I'd say the issue is with react-transition-group. I had the same issue when upgrading Lock. Probably a material-ui issue
image

@scamden
Copy link

scamden commented Feb 27, 2018 via email

@spunkedy
Copy link

got it, so it needs to be at least v2? I will chase that down then

@luisrudge
Copy link
Contributor Author

@scamden 11 was a new major, yes. The plan was to fix in v11, but we needed to address a security issue and wanted the migration from v10 to v11 to be as smooth as possible. Hopefully, we don't have to do this with v12 😝

@luisrudge
Copy link
Contributor Author

@spunkedy yes. v1 doesn't support react 16: reactjs/react-transition-group#199

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

Successfully merging this pull request may close these issues.

Using auth0-lock with React 16
5 participants