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

v1.10 and v2 umbrella #110

Open
8 tasks done
edorivai opened this issue Dec 12, 2018 · 43 comments
Open
8 tasks done

v1.10 and v2 umbrella #110

edorivai opened this issue Dec 12, 2018 · 43 comments

Comments

@edorivai
Copy link
Collaborator

edorivai commented Dec 12, 2018

Update (March 17th, 2019)

As described in detail in #123, I'm seriously considering publishing the current changes under a minor version bump: 1.10. The bulk of the preparations for this have already been done in #123.

The next step would be to implement a v2 which uses hooks for the core functionality. I expect a hooks implementation could simplify some of the internals, and would simultaneously allow us to ship a hook as part of the API.

2.0 1.10

2.1 2.0 (now available as react-media@next)

  • implement a hooks API, exposed as react-media/hooks
  • reimplement the core functionality using hooks, expose a useMedia hook
  • implement a <Media> component, which is compatible with v1.10, but uses said hook internally

I propose exposing Media as the default export and exporting the hook as a named export:

import Media, { useMedia } from 'react-media';

This would allow people to seamlessly upgrade to v2, without having to change existing code. There's also not really any use in splitting things up, since the Media component will be very small (just a wrapper around the hook), and it will use the hook under the hood. So I don't expect us splitting up the files will result in significant bundle size savings.

@tstirrat15
Copy link
Contributor

Want help with any of this?

@edorivai
Copy link
Collaborator Author

edorivai commented Dec 14, 2018

@tstirrat15 Feel free to take a stab at any of the last three - Strict mode tests, docs, hooks. I'm a bit short on time, but let me know if you want to start on one, and I'll write up some thoughts I have on that particular item.

@tstirrat15
Copy link
Contributor

Strict mode tests sounds the most immediately tractable. Is that pretty straightforward?

@edorivai
Copy link
Collaborator Author

Yes it should be. The idea would be to wrap all tests in <React.StrictMode>. @mjackson mentioned they're already doing this in react-router. Instead of calling ReactDOM.render directly, they pass their components under test to a renderStrict function they defined, which wraps it in StrictMode.

I don't see any actual assertions around Strict mode, but I guess it would be easily verifiable in the Travis runs as soon as something is violating strict mode.

Does that all make sense?

@tstirrat15
Copy link
Contributor

Yep.

What I've seen in using it in our own project is that it throws warnings at runtime. Is Travis configured to treat warnings as failures? Otherwise it'd probably entail manual review of the test output.

@edorivai
Copy link
Collaborator Author

Yeah I think manual review is fine. I usually run the tests before publishing, but more importantly, when users start complaining that something is not compliant with strict mode, we would be able to verify it quickly by running the tests.

@tstirrat15
Copy link
Contributor

Do you want/need any help with the docs? Did you have an idea for a direction on that?

@edorivai
Copy link
Collaborator Author

edorivai commented Jan 3, 2019

I already started on a cleanup of the docs on master. The general idea would be to merge master into next, and then update the breaking API change (query became queries). Though I have an outstanding conversation with @mjackson about whether we might want to keep the query prop in a backwards-compatible way, and just add the queries prop on top.

But I guess you could already perform the merge, and write up some docs on queries. See #72 for details.

Thanks again for the help man!

@tstirrat15
Copy link
Contributor

I'll write a doc block for queries as a separate PR, which we can then merge into next or master as needed. We can remove query and/or add clarification about how they'd work together as a final step.

@South-Paw
Copy link

South-Paw commented Jan 18, 2019

Would it possible to get an RC or alpha build released on npm for the next branch?

I'd really like to see if the 2 pass renderer solves some issues I'm seeing in a project.

Thanks

@edorivai
Copy link
Collaborator Author

@South-Paw Yeah, that would be possible. Only gotcha is that the next branch has a big breaking change, we replaced the query prop for queries. See #119 and #69 for details.

Would it still be useful for you to test out the next branch, given this breaking change?

@South-Paw
Copy link

Yeap I think it'd be fine to test it out even with those changes.

It won't be to hard to go through and migrate query to queries - from what I can see I can still put a string into queries and the render pattern remains as {matches => matches ? <div>a</div> : <div>b</div> } right?

@edorivai
Copy link
Collaborator Author

@South-Paw No, the queries prop has to be an object, where each of it's keys represents a media query:

<Media queries={{
    sm: "(max-width: 1000px)",
    lg: "(max-width: 2000px)"
  }}>
  {(matches) =>
    <>
      { matches.sm && <span>I'm small</span>}
      { matches.lg && <span>I'm large</span>}
    </>
  }
</Media>

@South-Paw
Copy link

Okay, that looks good - liking that pattern as well 👍

If it's possible to do a alpha/RC release that'd be awesome - I'll give it a try out and see how it goes

@edorivai
Copy link
Collaborator Author

@South-Paw Hey I just prepared everything for an alpha release, but sadly the deploy process halted due to an invalid npm token. I don't have access to the npm project, so we'll have to wait for @mjackson to fix the token in Travis. See #120 for details.

@South-Paw
Copy link

Hi, its been 2 weeks and @mjackson hasn't responded to #120 - has there been any movement on this?

Still pretty keen to try test this 👍

@philhradecs
Copy link

philhradecs commented Feb 2, 2019 via email

@edorivai
Copy link
Collaborator Author

edorivai commented Feb 4, 2019

Hey guys, I'm really sorry about the delay. I've dm'd @mjackson, but I imagine his inboxes must be bursting 😅.

In the meantime, would it be an option for you guys to test the next branch locally? You can check out a fork of this repo, then:

  • check out the next branch
  • run npm run build
  • run npm link
  • inside your own project: npm link react-media

Now run your project as usual, it should use your local copy of react-media.

@philhradecs
Copy link

Thank you!
I will try that later today.
Awesome work :)

@philhradecs
Copy link

During build I get the following error:
I thought I posted this in case it's relevant for the upcoming release.
I dug into the error a bit but this is not something I can solve as of yet..

modules/index.js → esm/react-media.js... [!] (size-snapshot plugin) Error: ModuleNotFoundError: Module not found: Error: Can't resolve './Media.js' in '/' Error: ModuleNotFoundError: Module not found: Error: Can't resolve './Media.js' in '/' at compiler.run (C:\_code\projects\react-media\node_modules\rollup-plugin-size-snapshot\dist\treeshakeWithWebpack.js:58:16) [.. further traceback]

@edorivai
Copy link
Collaborator Author

edorivai commented Feb 4, 2019

@baumzeit Huh, I did the following, and was able to successfully build the project:

  • Checkout latest version of the next branch
  • Run npm ci to install dependencies
  • Run npm run build

Did you do something similar?

@philhradecs
Copy link

philhradecs commented Feb 4, 2019

Yes, I can confirm:

  • forked react-media
  • cloned the repo
  • checkout next
  • npm install (also tried ci)
  • npm run build

throws the above error.

Here is the complete trace:

Error: Module not found: Error: Can't resolve './Media.js' in '/'

at compiler.run (C:\_code\projects\react-media\node_modules\rollup-plugin-size-snapshot\dist\treeshakeWithWebpack.js:58:16)
at finalCallback (C:\_code\projects\react-media\node_modules\webpack\lib\Compiler.js:210:39)
at hooks.done.callAsync.err (C:\_code\projects\react-media\node_modules\webpack\lib\Compiler.js:226:13)
at AsyncSeriesHook.eval [as callAsync] (eval at create (C:\_code\projects\react-media\node_modules\tapable\lib\HookCodeFactory.js:32:10), <anonymous>:6:1)
at AsyncSeriesHook.lazyCompileHook (C:\_code\projects\react-media\node_modules\tapable\lib\Hook.js:154:20)
at onCompiled (C:\_code\projects\react-media\node_modules\webpack\lib\Compiler.js:224:21)
at hooks.afterCompile.callAsync.err (C:\_code\projects\react-media\node_modules\webpack\lib\Compiler.js:552:14)
at AsyncSeriesHook.eval [as callAsync] (eval at create (C:\_code\projects\react-media\node_modules\tapable\lib\HookCodeFactory.js:32:10), <anonymous>:6:1)
at AsyncSeriesHook.lazyCompileHook (C:\_code\projects\react-media\node_modules\tapable\lib\Hook.js:154:20)
at compilation.seal.err (C:\_code\projects\react-media\node_modules\webpack\lib\Compiler.js:549:30)
at AsyncSeriesHook.eval [as callAsync] (eval at create (C:\_code\projects\react-media\node_modules\tapable\lib\HookCodeFactory.js:32:10), <anonymous>:6:1)
at AsyncSeriesHook.lazyCompileHook (C:\_code\projects\react-media\node_modules\tapable\lib\Hook.js:154:20)
at hooks.optimizeAssets.callAsync.err (C:\_code\projects\react-media\node_modules\webpack\lib\Compilation.js:1323:35)
at AsyncSeriesHook.eval [as callAsync] (eval at create (C:\_code\projects\react-media\node_modules\tapable\lib\HookCodeFactory.js:32:10), <anonymous>:6:1)
at AsyncSeriesHook.lazyCompileHook (C:\_code\projects\react-media\node_modules\tapable\lib\Hook.js:154:20)
at hooks.optimizeChunkAssets.callAsync.err (C:\_code\projects\react-media\node_modules\webpack\lib\Compilation.js:1314:32) 

[... npm error messages]

@edorivai
Copy link
Collaborator Author

edorivai commented Feb 5, 2019

@baumzeit Damn, you think this could be a windows issue? I don't have a windows environment at hand to test :\

In any case, when the build get's fixed you should be able to test the alpha release. Sorry again for the wait

@philhradecs
Copy link

@edorivai It looks like it's a windows issue.
I tested it out on another windows machine with the same result.

  • checkout next
  • npm ci
  • npm run build

@South-Paw
Copy link

South-Paw commented Mar 10, 2019

I've gotten really frustrated waiting for a month and a half when I really need to test and/or fix the SSR issue a few Gatsby websites now...

I'm going through building and publishing my own version of react-media and hit the same issue as @baumzeit on Windows.

Problem seems to be in the rollup-plugin-size-snapshot package. Updating it to v0.8.0 changed the error to

[!] (size-snapshot plugin) TypeError: Cannot read property 'find' of undefined
TypeError: Cannot read property 'find' of undefined
    at then.then (E:\dev\Github\react-media\node_modules\rollup-plugin-size-snapshot\dist\treeshakeWithRollup.js:67:36)

Can't tell if its required or not, but removing the plugin from the rollup.config.js allows the package to build again.

@South-Paw
Copy link

Scratch that, doesn't seem my republished version works either.

 ERROR  Failed to compile with 1 errors

This relative module was not found:

* ./Media.js in ./node_modules/@south-paw/react-media/esm/react-media.js
× 「wdm」:
ERROR in ./node_modules/@south-paw/react-media/esm/react-media.js
Module not found: Error: Can't resolve './Media.js' in 'E:\dev\Github\...\node_modules\@south-paw\react-media\esm'
 @ ./node_modules/@south-paw/react-media/esm/react-media.js 1:0-34 2:0-37 2:0-37
 @ ./src/templates/Site.js
 @ ./src/pages/acceptable-use-policy.js
 @ ./.cache/sync-requires.js
 @ ./.cache/app.js
 @ multi event-source-polyfill (webpack)-hot-middleware/client.js?path=/__webpack_hmr&reload=true&overlay=false E://dev//Github//...//.cache//app
i 「wdm」: Failed to compile.

How long until the npm token gets fixed @edorivai? I've pinged @mjackson twice on twitter and I doubt he's seen either... but its getting really frustrating that it's been a month and a half with no movement on the issue.

@edorivai
Copy link
Collaborator Author

@South-Paw Hey man, I understand that you're frustrated. I've pinged @mjackson on twitter, as well as email, so there isn't too much I can do anymore. I've also considered publishing under a different npm package, but honestly didn't get the time to do so yet.

One quick remark on the error you're running into. When I build on my system (I'm running Linux), there's only a single file inside the esm directory (see screenshot below). Reading your error message, it seems to me that the esm/react-media.js file is trying to import the ./Media.js file, which is weird, since that file should be rolled up into react-media.js. Could you maybe tell me what the files inside your esm directory look like?

image

@South-Paw
Copy link

@edorivai Sorry, I know its not your fault and @mjackson is obviously a busy person as well - just annoying to have a ~70k dl/w package, that works really really well and has a good 2.0 update coming to be hamstrung by a such a trivial issue 😥

I've been out tonight so no code time, but I'll run another build tomorrow and take a screenshot. Thanks for the continued replies 👍

@moflo
Copy link

moflo commented Mar 13, 2019

Hi @edorivai - we needed a hooks port so we created one, here: react-media-hooks. I don't plan to publish to npm as this would likely be in conflict with your 2.0 plans. Also, we would need to add more tests, TS support, etc., but this private version works for us. Thanks!

@edorivai
Copy link
Collaborator Author

@moflo Thanks 😄. We're planning to add hooks as well, but we first want to get v2 published.

@South-Paw @baumzeit Good news, the deployment problems have been fixed 🎉. You should be able to install react-media@next. Let me know how it goes!

@South-Paw
Copy link

@edorivai awesome, thanks 👍

@edorivai edorivai changed the title v2 umbrella v1.10 and v2 umbrella Mar 17, 2019
@South-Paw
Copy link

@edorivai gave react-media@next a run on the Gatsby website I needed it on - unfortunately the 2 pass renderer that I thought would solve the issue that I'm seeing didn't make a difference when I built the site for deployment 😞

While I don't think I've got a specific Gatsby issue, I do think its to do with how Gatsby statically creates the website for build/publish and how it interacts with react-media or possibly styled-components inside of react-media

Unsure what steps I can take next on this 🤔 so any suggestions are welcome

@edorivai
Copy link
Collaborator Author

@South-Paw If you have the time, it would be incredibly helpful if you could post a minimal reproduction repository that I can try out for myself.

One thing I can add though, I took a look with @baumzeit at the problems he was having, and he was using react-static. This was before the two-pass render was published to npm, so we quickly implemented the two-pass render in his own codebase. And that did seem to solve his problem (inconsistent DOM after rehydration).

@edorivai
Copy link
Collaborator Author

@baumzeit If you have time, would you be so kind to test react-media@next in your project, and remove the custom fix for your two-pass rendering issue?

@tstirrat15
Copy link
Contributor

Does that mean that queries is available in @next and that we should go ahead and give it a go?

@edorivai
Copy link
Collaborator Author

It does, so yes, please try it out😊

@joefiorini
Copy link

@edorivai I had to write a component that could listen on multiple, device-specific media queries, so I went ahead and used this alpha. It's working great in our uses of a single query and our new use of multiple queries. The one downside is that now the typescript defs are out-of-date. I tried my hardest to write a type that would tie the keys from the queries prop to the keys in the match prop, but couldn't figure it out in a way that would work with a union (or conditional type) to also allow a single query prop. If I can figure it out I'll open a PR, but need to move on for now.

@edorivai
Copy link
Collaborator Author

@joefiorini Thanks for letting me know! I'll have to take a look at the TS definitions

@deasems
Copy link

deasems commented May 8, 2020

What's the status of 2.0 and hooks support?

@tstirrat15
Copy link
Contributor

I can put through a PR for the hook update.

@abenhamdine
Copy link
Contributor

abenhamdine commented Jun 13, 2020

For any one interested : hooks are available in 2.X branch, with npm install react-media@next
It's a RC version but it works perfectly for us.

Big big thx to @tstirrat15 and @edorivai for this ! ❤️

@JonasJuss
Copy link

JonasJuss commented Jun 22, 2020

I just tested react-media@next with my gatsby website and I can confirm that the two-pass rendering does not seem to work anymore. My media query is set to render the content for mobile in the SSR and when I open the website on desktop it does not change but stays on the mobile version.

I had to revert back to the 1.10.0 to make everything work as intended again.

@tstirrat15
Copy link
Contributor

@JonasJuss can you give me an example of how it's supposed to work and how it fails? I'll admit that I don't understand two-pass rendering well enough, and it hasn't yet been captured in a test. I'd like to write one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

9 participants