Add hooks implementation

tstirrat15 commented 4 years ago

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.

tstirrat15 commented 4 years ago

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 commented 4 years ago

This should be ready for review now.

edorivai commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Now available at react-media@next :tada: 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 :pray:

abenhamdine commented 4 years ago

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.

ReactTraining / react-media