RMIZ's future: React 16 & beyond RFC

rpearce commented 5 years ago

Intro

With the latest React version and React's future as a whole, some things have come to light that need changing, and this has me thinking that it's time for an overhaul on this project.

Current State

While the project works fine as it is right now, here are some observations:

mixing the controlled vs uncontrolled component modes makes for some very messy code, and it results in some people requiring in code that has nothing to do with how they use the component
current lifecycle methods need to be changed & future-proofed (as well as one can)
there exists a currently seemingly insurmountable issue (https://github.com/rpearce/react-medium-image-zoom/issues/136) that is something I'm afraid I can't address with how things are built right now and feel I can't rip everything apart because...
there are no tests (https://github.com/rpearce/react-medium-image-zoom/issues/55)
keyboard accessibility is a bit hacky at the moment

The Future?

Split Controlled & Uncontrolled Components

For this, I'm thinking either 1 of 2 things:

offer up different components to import or
go the monorepo route and offer them up as different packages

I could use some feedback on this.

All the "business logic" around what makes these components tick would naturally be extracted, shared and tested. Unlike input components, which can be controlled or uncontrolled (given you provide a value or not), developers know from the get-go how they're going to use this component, so I'm of a mind to have them choose from the start.

Updates for future React

componentWillMount, componentWillRecieveProps, componentWillUpdate are all about to be deprecated, so those need to either be updated to something else or find a way around them. Here's a blog post from Dan Abramov about what, why and what's coming: https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html. It's also listed as erroring as part of the new StrictMode component: https://reactjs.org/docs/strict-mode.html#identifying-unsafe-lifecycles.
should update all refs to use the new React.createRef() API: https://reactjs.org/docs/refs-and-the-dom.html. Here's the StrictMode section on it: https://reactjs.org/docs/strict-mode.html#warning-about-legacy-string-ref-api-usage.
...consider using hooks here? bad for supporting older versions? but then again this is supposed to be a rewrite for future react?
...what else?

Testing

I've gotten a lot better about testing React components since this was initially published and updates made. I might as well use this as an opportunity to test all the things instead of relying on Storybook.

Keyboard a11y

Not much to say here other than it seems a bit wonky across browsers. Can address more fully if it's baked in from the get-go.

General Cleanliness

Parts of this library are quite difficult to follow and are hacky because it's what I had to do to make the library work. That said, I think this could improve the quality of the code (and dependability with tests) and make it easier for others to contribute.

Portability Across Frameworks / Languages

I'd love to identify patterns and code blocks that can be used as inspiration for porting this to

vanilla JS
elm
purescript
vue (which I know nothing about)
...? if I or someone else would like to.

Docs

Small point, but I'm going to move the documentation to separate markdown files and keep the README clean and to the point:

API.md
CONTRIBUTING.md
whatever else is needed

Thoughts? I'm asking all of you because you have contributed in this project in some way and should have a say: cc @cbothner @jeremybini @ismay @rajit @rsaccon @wtfdaemon @aswinckr @alexshelkov @Snowsoul @kendagriff @HippoDippo @oyeanuj @ludwigfrank @serfgy @babyPrince

cbothner commented 5 years ago

I’m in favor of exporting multiple components from the same package, rather than splitting into two packages. It’s likely a user will start with the stateful component and then grow into needing the lower level controlled component. My thinking is that it’s lower friction and more discoverable to change the import at the top of the file rather than package.json.

rpearce commented 5 years ago

@cbothner thank you for the feedback!

rpearce commented 5 years ago

Update: I've now broken ground on this. Here we go... Working from the rewrite branch

rpearce commented 5 years ago

Update: This has been paused as I've been having hand+wrist+forearm pain and need to deal with that before doing this outside normal working hours.

Good news is that we'll be using latest & greatest React & hooks!

rpearce commented 5 years ago

@spencerfdavis is going to be giving me a hand with this. Welcome to the project, @spencerfdavis, and I look forward to getting some things rolling together over the coming months!

rpearce commented 5 years ago

Update: I've been putting a bit of work into the rewrite and am having a meeting with @spencerfdavis on Monday to figure out what path we want to go down with regard to some things.

We're going to

plan everything that needs doing out (and how ... this is interesting)
create a project board
break things into issues and place them on that board
use https://github.com/rpearce/react-medium-image-zoom/pull/159 as the main rewrite branch until we have something that solves all outstanding problems & we determine a good API. Note: what is there right now is a prototype for ideas around what we're trying to accomplish.

The ultimate goals are:

accessibility
simplify, be smarter, much less code
100% test coverage
easily work with the styles (that rewrite branch has me doing styleInject via rollup, but I've come to learn that content security policies do not jive with that sort of thing well...we may just output a .css file...TBD)
ensure all outstanding issues are handled (that includes mobile)
- *

There are also a few other folks who have expressed interest in contributing (thanks, Hacktoberfest!), so more to come on that, as well.

rpearce commented 5 years ago

The prototype for the rewrite is out on the master branch (master is dev for now), and it needs a lot of work. We'll be tracking its progress in https://github.com/rpearce/react-medium-image-zoom/projects/1, so I'm going to close this issue.

rpearce / react-medium-image-zoom