fix #103. update dependencies #104
Conversation
related to #3 update for React 16 and 17
LICENSE.md
Outdated
| @@ -1,6 +1,6 @@ | |||
| BSD 3-Clause License | |||
|
|
|||
| Copyright (c) 2019, Indiana University | |||
| Copyright (c) 2021, Indiana University | |||
There was a problem hiding this comment.
This should stay as 2019. It should be the year in which the project was open sourced, not the current year.
examples/timer/.babelrc
Outdated
| @@ -1,3 +1,4 @@ | |||
| { | |||
| "presets": ["env", "react"] | |||
| "plugins": ["@babel/plugin-proposal-object-rest-spread"], | |||
There was a problem hiding this comment.
This plugin should already be included in the latest @babel/preset-env. So, try removing it.
examples/timer/package.json
Outdated
| "@babel/core": "7.16.0", | ||
| "@babel/plugin-proposal-object-rest-spread": "7.16.0", | ||
| "@babel/preset-env": "7.16.4", | ||
| "@babel/preset-react": "7.16.0", |
There was a problem hiding this comment.
There are newer patch versions of these dependencies. Please update.
examples/todomvc/package.json
Outdated
| "@babel/core": "7.16.0", | ||
| "@babel/plugin-proposal-object-rest-spread": "7.16.0", | ||
| "@babel/preset-env": "7.16.4", | ||
| "@babel/preset-react": "7.16.0", |
There was a problem hiding this comment.
There are newer patch versions of these dependencies. Please update.
examples/word-count/package.json
Outdated
| "@babel/core": "7.16.0", | ||
| "@babel/plugin-proposal-object-rest-spread": "7.16.0", | ||
| "@babel/preset-env": "7.16.4", | ||
| "@babel/preset-react": "7.16.0", |
There was a problem hiding this comment.
There are newer patch versions of these dependencies. Please update.
packages/conduit-rxjs/package.json
Outdated
| "@babel/core": "7.16.0", | ||
| "@babel/plugin-proposal-object-rest-spread": "7.16.0", | ||
| "@babel/preset-env": "7.16.4", |
There was a problem hiding this comment.
There are newer patch versions of these dependencies. Please update.
| "@babel/core": "7.16.0", | ||
| "@babel/plugin-proposal-object-rest-spread": "7.16.0", | ||
| "@babel/preset-env": "7.16.4", | ||
| "@babel/preset-react": "7.16.0", |
There was a problem hiding this comment.
There are newer patch versions of these dependencies. Please update.
|
|
||
| 1. Updated dependencies to latest version. | ||
|
|
||
| 2. Updated React lifecycle method name for use with React 16.3 to 18.x. This is a temporary fix as opposed to a more permanent fix discussed in [Issue #3](https://github.com/indiana-university/conduit/issues/3). |
There was a problem hiding this comment.
Are we sure that it is "to 18.x" and not "to 17.x"? I couldn't find a source one way or another. But it seems odd that the UNSAFE methods would carry over to a second major version.
examples/todomvc/webpack.config.js
Outdated
| 'router': 'Router', | ||
| 'rxjs': 'rxjs', | ||
| router: 'Router', | ||
| rxjs: 'rxjs', | ||
| 'rxjs/operators': 'rxjs.operators' |
There was a problem hiding this comment.
As of RxJS v7.2 (2021-07-05):
Operators are all exported at the top level, from "rxjs". From here on out, we encourage top-level imports with RxJS. Importing from rxjs/operators will be deprecated soon.
So, if we upgrade the examples to the latest RxJS, then we can clean up this "externals" line — here and in all the examples. We could also force the minimum RxJS version to be v7.2, if we wanted. It would primarily affect conduit-rxjs-react since it uses operators, while conduit-rxjs does not. But if we force a minimum version on one, we may as well do the same for the other.
There was a problem hiding this comment.
👍 Since we're forcing React updates we might as well also force RxJS updates.
There was a problem hiding this comment.
Would it be appropriate to replace the references to rxjs/operators in the CHANGELOG.md files too?
There was a problem hiding this comment.
The README files and examples should be updated. But operator imports mentioned in prior releases in the CHANGELOG files should be kept as-is. It should reflect the code at that point in time. This change should be mentioned in the CHANGELOG for this upcoming version.
There was a problem hiding this comment.
Since all operators are now a top-level import, I'd have one import line, rather than two.
// Update this:
import { interval } from 'rxjs'
import { map, startWith, switchMap } from 'rxjs/operators'
// To this (organizing imports alphabetically):
import { interval, map, startWith, switchMap } from 'rxjs'
|
|
||
| 2. Updated React lifecycle method name for use with React 16.3 to 17.x. This is a temporary fix as opposed to a more permanent fix discussed in [Issue #3](https://github.com/indiana-university/conduit/issues/3). | ||
|
|
||
| 3. Switched `rxjs/operators` imports to `rxjs` since they moved to top level in RxJS v7.2. |
There was a problem hiding this comment.
This could be updated with a direct link.
3. Switched `rxjs/operators` imports to `rxjs` since they moved to top level in [RxJS v7.2](https://github.com/ReactiveX/rxjs/blob/master/CHANGELOG.md#720-2021-07-05).
add package-lock.json to .gitignore add link to RxJS v7.2 CHANGELOG
|
|
||
| 2. Updated React lifecycle method name for use with React 16.3 to 17.x. This is a temporary fix as opposed to a more permanent fix discussed in [Issue #3](https://github.com/indiana-university/conduit/issues/3). | ||
|
|
||
| 3. Switched `rxjs/operators` imports to `rxjs` since they moved to top level in [RxJS v7.2](https://github.com/ReactiveX/rxjs/blob/master/CHANGELOG.md#720-2021-07-05). |
There was a problem hiding this comment.
I want to be more explicit about the new peer dependency version. Also, make this item 2, rather than item 3 in this log. That way, this conduit-rxjs-react log will start identically to the conduit-rxjs log.
2. Increased minimum peer dependency version of [RxJS to v7.2](https://github.com/ReactiveX/rxjs/blob/master/CHANGELOG.md#720-2021-07-05). Operators are now imported from the top-level `rxjs` module, since importing from `rxjs/operators` will be depreciated soon.
There was a problem hiding this comment.
Also, the examples are broken. The index.html files need updated versions. I'd recommend React be v16.3.0, since that's the minimum version we're asking for. And likewise, RxJS be v7.2.0. Also, note that the bundle folder for RxJS has changed. It's now under dist.
<script crossorigin src="https://unpkg.com/[email protected]/umd/react.production.min.js"></script>
<script crossorigin src="https://unpkg.com/[email protected]/umd/react-dom.production.min.js"></script>
<script crossorigin src="https://unpkg.com/[email protected]/dist/bundles/rxjs.umd.min.js"></script>
|
Once this PR merges, perhaps we do one minor PR to update the publish dates in the change logs. Then we publish. These are all plenty of updates for this next release. |
|
@basham, I've got the most recent requested changes done. I'm good with doing a minor PR to set the publish dates just before publishing. |
|
@james-anderson The examples all work. It looks good to me. Thanks for all your work on this! I'll leave it to you to merge, whenever you're ready to do so. Do you want to work on the next PR and publish the changes to npm? You should still have the ability to do the publishing, if you want to do that. If not, I can take care of it. Up to you. |
|
@basham I can do the publish date PR tomorrow and publish following the CONTRIBUTING.md steps. |
Related to #3 update for React 16 and 17
In testing a project using React 16.4 and 17.0, I found that I had to update Babel dependencies and
.babelrcfiles in those projects to get the project to compile with theconduitupdates. It is possible it was just the way I went about testing it or that the React 16.4 project had other issues with its dependencies. If it's not just me and the projects I tested with, we may want to include the Babel dependencies inpeerDependenciesto produce a warning onnpm installrather than compile errors when building the project. We would probably want to update thepeerDependenciesfor React (included in this PR) due to the function name change ofcomponentWillReceivePropstoUNSAFE_componentWillReceivePropsinconduit-rxjs-react. I considered including both versions of the function name thinking older versions of React would notice the old name and ignore the new one and newer versions would ignore the old one and use the new one, but React was throwing warnings about the old name since it will still be recognized until v18, so I just replaced it. It seems like the RxJSpeerDependenciescan stay as-is.