Add hooks implementation #149

tstirrat15 · 2020-05-10T19:47:41Z

Fixes #148

Motivation

This has been requested functionality for a while now and I found the time to get around to it.

Changes

I'll annotate the files, since this diff is pretty noisy.

Notes

I'm not entirely sure that the two-pass SSR render behavior has been preserved. I'd love some help figuring that out in code review.

This should be released as a major version according to semantic versioning rules because peer dependencies are changing. It also ended up being a pretty major rewrite.

…erser

tstirrat15

See comments. Please let me know if they're unclear, or if there's additional explanation desired.

tstirrat15 · 2020-05-10T19:48:18Z

.eslintrc

@@ -3,18 +3,19 @@
  "env": {
    "browser": true
  },
-  "plugins": ["import", "react"],
+  "plugins": ["import", "react", "react-hooks"],


There's a separate ESLint plugin for the rules of hooks, which will be nice for anyone working on this library.

tstirrat15 · 2020-05-10T19:48:49Z

modules/Media.js

 import PropTypes from "prop-types";
 import invariant from "invariant";
 import json2mq from "json2mq";

 import MediaQueryListener from "./MediaQueryListener";

-const queryType = PropTypes.oneOfType([


Pushed these down to live with the new location for propTypes.

tstirrat15 · 2020-05-10T19:49:17Z

modules/Media.js

-  PropTypes.object,
-  PropTypes.arrayOf(PropTypes.object.isRequired)
-]);
+const checkInvariants = ({ query, queries, defaultMatches }) => {


There wasn't a strong reason to pull this out into a separate function - it mostly just seemed noisy in the constructor.

tstirrat15 · 2020-05-10T19:50:02Z

modules/Media.js

-      !(!props.query && !props.queries) || (props.query && props.queries),
-      '<Media> must be supplied with either "query" or "queries"'
-    );
+const wrapInQueryObject = query => ({ __DEFAULT__: query });


Moved this and unWrapSingleQuery up to the top, since I refactored them as arrow functions. Again, that's more a stylistic preference; if there's a strong reason to leave them as functions I can refactor.

tstirrat15 · 2020-05-10T19:52:38Z

modules/Media.js

-    const result = this.queries.reduce(
+export const useMedia = ({ query, queries, defaultMatches, targetWindow, onChange }) => {
+  checkInvariants({ query, queries, defaultMatches });
+  const activeQueries = useRef([]);


This provides a stable location for the media queries to live from render to render. I could have used state for this as well, but I figured that since there isn't logic to change query or queries from render to render, having logic to make activeQueries change from render to render wouldn't have made sense.

Actually, now that I think about it, was it an intentional design decision to not allow for the quer(y/ies) props to change from render to render, e.g. in a componentDidUpdate? I can refactor if that would be desired behavior.

tstirrat15 · 2020-05-10T20:23:02Z

package.json

@@ -39,33 +39,32 @@
    "@babel/plugin-transform-runtime": "^7.1.0",
    "@babel/preset-env": "^7.1.6",
    "@babel/preset-react": "^7.0.0",
+    "@rollup/plugin-babel": "5.0.0",


npm audit complained about rollup-plugin-snapshot-size and upgraded it beyond a version that worked with the current version of rollup, so I upgraded Rollup and all its friends.

tstirrat15 · 2020-05-10T20:23:21Z

rollup.config.js

 import { sizeSnapshot } from 'rollup-plugin-size-snapshot';
-import { uglify } from 'rollup-plugin-uglify';
+import { terser } from 'rollup-plugin-terser';


Uglify became Terser. These are otherwise functionally identical packages.

tstirrat15 · 2020-05-10T20:23:56Z

rollup.config.js

@@ -18,35 +18,35 @@ function external(id) {
 const cjs = [
  {
    input,
-    output: { file: `cjs/${pkg.name}.js`, format: 'cjs' },
+    output: { file: `cjs/${pkg.name}.js`, format: 'cjs', exports: 'named' },


Rollup wants you to be explicit about named exports when you're providing both a default and named export. It still complains if you provide both a default and a named export, but we're ignoring that for now.

tstirrat15 · 2020-05-10T20:24:32Z

rollup.config.js

    external,
    plugins: [
-      babel({ exclude: /node_modules/ }),
+      babel({ exclude: /node_modules/, babelHelpers: 'bundled' }),


The new version of the babel plugin uses a different setting for whether to include the babel runtime, so this changed throughout the fiel.

tstirrat15 · 2020-05-10T20:25:15Z

.size-snapshot.json

      },
      "webpack": {
-        "code": 4345
+        "code": 3673


Check it out! We're actually getting a smaller package out of this.

tstirrat15 · 2020-05-10T21:08:17Z

package.json

  },
  "dependencies": {
-    "@babel/runtime": "^7.2.0",
+    "@babel/runtime": "^7.9.6",


Will this cause problems? Or will it just ensure that the runtime required by an end-user ends up being at least this recent?

tstirrat15 · 2020-05-11T15:07:19Z

I was running into this error:

 FAIL  modules/__tests__/Media-test.js
  ● Test suite failed to run

    Requires Babel "^7.0.0-0", but was loaded with "6.26.3". If you are sure you have a compatible version of @babel/core, it is likely that something in your build process is loading the wrong version. Inspect the stack trace of this error to look for the first entry that doesn't mention "@babel/core" or "babel-core" to see what is calling Babel.

      at throwVersionError (node_modules/@babel/helper-plugin-utils/lib/index.js:65:11)
      at Object.assertVersion (node_modules/@babel/helper-plugin-utils/lib/index.js:13:11)
      at node_modules/@babel/plugin-proposal-export-default-from/lib/index.js:17:7
      at node_modules/@babel/helper-plugin-utils/lib/index.js:19:12
          at Array.map (<anonymous>)

And couldn't figure out what to make of it. I found a bunch of issues that talked about it:

but none of the fixes detailed in those issues seemed to work. I tried swapping yarn for npm on a whim, and it started working for reasons that I can't grok.

Any objections to using Yarn?

tstirrat15 · 2020-05-11T15:07:39Z

This should be ready for review now.

edorivai · 2020-05-13T12:01:37Z

Good catch on trying yarn. I suppose there is some locked package that imports the wrong babel version for some reason. So I deleted the package-lock.json and re-ran npm i. After that npm test runs successfully, it does show some errors in the console though, I think this is expected. Perhaps you could confirm?

I actually think library authors shouldn't use package-lock.json since users won't be applying our lockfile when importing react-media.

> jest

 PASS  modules/__tests__/Media-ssr-test.js
 PASS  modules/__tests__/Media-test.js
  ● Console

    console.error node_modules/jsdom/lib/jsdom/virtual-console.js:29
      Error: Uncaught [Invariant Violation: <Media targetWindow> does not support `matchMedia`.]
          at reportException (/home/edo/work/react-media/node_modules/jsdom/lib/jsdom/living/helpers/runtime-script-errors.js:66:24)
          at invokeEventListeners (/home/edo/work/react-media/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:209:9)
          at HTMLUnknownElementImpl._dispatch (/home/edo/work/react-media/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:119:9)
          at HTMLUnknownElementImpl.dispatchEvent (/home/edo/work/react-media/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:82:17)

...more truncade stack traces

edorivai · 2020-05-13T12:05:02Z

Pinging @mjackson to give him a chance to review this if he has time. If he doesn't jump in we'll release an alpha soon.

tstirrat15 · 2020-05-13T15:03:05Z

Ah, yeah. Looks like you're correct about lockfiles: https://dev.to/gajus/stop-using-package-lock-json-or-yarn-lock-3ddi

I'll remove the lockfile from this PR and see how it works.

I'll also do some research on silencing that error. I think it's in that test case that's designed to throw, and there's probably something I need to do in Jest to make sure that the error is fully caught.

tstirrat15 · 2020-05-13T15:11:42Z

I've made both of those changes. I think there's some additional refactoring and simplification that could be done, but I'd be okay with leaving that for future work.

edorivai · 2020-05-15T16:52:40Z

Hey man, just letting you know I haven't forgotten about this! Life a bit hectic ATM, I'll do my best to squeeze this in ASAP.

tstirrat15 · 2020-05-15T17:11:48Z

No worries! You've already been more present and responsive than multiple projects that I've submitted PRs to in recent months. Thank you!

edorivai

This looks good, thank you so much man. There are a two points of improvements that we should add, but I want to merge this in and release an alpha tag anyway. I'll open a new issue (#150) for the improvements:

Add tests for useMedia()
Add documentation for useMedia()
Add types to index.d.ts for useMedia()

edorivai · 2020-05-27T09:54:01Z

Now available at react-media@next 🎉
I've released it as a major version bump, because of the required react version 16 upgrade.

It would be great if some people could test it out on their codebases and report back any issues 🙏

abenhamdine · 2020-06-10T12:09:42Z

great great news !
So glad to see that and thx again for all your hard work.
We will made some tests in our new application and let you know if there are some issues.

tstirrat15 added 17 commits May 9, 2020 07:14

npm audit fix

593f125

Bump react versions, add hooks eslint

997777c

Update package.lock

e1436ee

Add hook rules to eslint

dfc2b2c

Add testing library to project

21a65f5

First pass with all tests working

e5935cf

Refactor tests to use testing-library patterns

3fc6049

Refactor renderStrict to use testing-library

92f7c72

Make nested ternaries explicit using parentheses

1444394

Extract hook logic and export new function

e18f27c

Add useMedia to exports

c489cbb

Explicitly tell rollup to use named exports

97c1d0c

Remove TODO

9459522

Add exports: named, be explicit about babelHelpers, swap uglify for t…

02602bb

…erser

Upgrade rollup and friends to latest

a59d672

Upgrade rollup and friends to latest

5f89897

Check it out! smaller bundles

f491714

tstirrat15 commented May 10, 2020

View reviewed changes

tstirrat15 added 4 commits May 10, 2020 14:31

Remove unused class property babel transform

8619b1e

Bump babel-jest version, remove now-unused transformation

a1e5d9e

Update size snapshot

37c7086

Upgrade the babels too

fe58da4

tstirrat15 commented May 10, 2020

View reviewed changes

tstirrat15 added 5 commits May 11, 2020 08:39

Add babel bridge for jest's benefit

0a30e06

Switch from npm to yarn

49758b4

Mild reformatting of babelrc

0c5e83d

move travis.yml to use yarn

655b58a

Fix proptypes errors

8cefd6f

tstirrat15 added 3 commits May 13, 2020 09:03

Remove lockfile

1144c64

Turn off package.lock for the repository

256b02c

Prevent jest errors from showing up in stderr output

13cd76d

zjhch123 mentioned this pull request May 21, 2020

Media tries to update unmounted component #139

Open

edorivai approved these changes May 27, 2020

View reviewed changes

edorivai merged commit 410b4af into ReactTraining:master May 27, 2020

edorivai mentioned this pull request May 27, 2020

Improve docs and tests for useMedia hook #150

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hooks implementation #149

Add hooks implementation #149

tstirrat15 commented May 10, 2020 •

edited

Loading

tstirrat15 left a comment

tstirrat15 May 10, 2020

tstirrat15 May 10, 2020

tstirrat15 May 10, 2020

tstirrat15 May 10, 2020

tstirrat15 May 10, 2020

tstirrat15 May 10, 2020

tstirrat15 May 10, 2020

tstirrat15 May 10, 2020

tstirrat15 May 10, 2020

tstirrat15 May 10, 2020

tstirrat15 May 10, 2020

tstirrat15 commented May 11, 2020

tstirrat15 commented May 11, 2020

edorivai commented May 13, 2020

edorivai commented May 13, 2020

tstirrat15 commented May 13, 2020

tstirrat15 commented May 13, 2020

edorivai commented May 15, 2020

tstirrat15 commented May 15, 2020

edorivai left a comment •

edited

Loading

edorivai commented May 27, 2020 •

edited

Loading

abenhamdine commented Jun 10, 2020

Add hooks implementation #149

Add hooks implementation #149

Conversation

tstirrat15 commented May 10, 2020 • edited Loading

Motivation

Changes

Notes

tstirrat15 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstirrat15 commented May 11, 2020

tstirrat15 commented May 11, 2020

edorivai commented May 13, 2020

edorivai commented May 13, 2020

tstirrat15 commented May 13, 2020

tstirrat15 commented May 13, 2020

edorivai commented May 15, 2020

tstirrat15 commented May 15, 2020

edorivai left a comment • edited Loading

Choose a reason for hiding this comment

edorivai commented May 27, 2020 • edited Loading

abenhamdine commented Jun 10, 2020

tstirrat15 commented May 10, 2020 •

edited

Loading

edorivai left a comment •

edited

Loading

edorivai commented May 27, 2020 •

edited

Loading