mrousavy / react-native-blurhash

🖼️ A library to show colorful blurry placeholders while your content loads.
https://blurha.sh
MIT License
1.98k stars 70 forks source link

WIP|Implement for react-native-web #112

Closed jacob-israel-turner closed 3 months ago

jacob-israel-turner commented 3 years ago

I'd also like to cleanup the commit history before this merges.

jacob-israel-turner commented 3 years ago

@mrousavy looks like the linting step failed because package-lock.json has gotten out of sync. Would you like me to use yarn.lock, package-lock.json, or both?

jacob-israel-turner commented 3 years ago

Received the following from @mrousavy via email, figured it would make sense to reply here:

I don't think you should install expo

Do note - I only added expo to the example repo. The library is unaffected. I did this, because setting up a raw react-native-web project is fairly involved. I spent about an hour doing this before I jumped into some other example repos on react-native-web -compatible libraries. Specifically, you linked to react-native-slider as a pattern to build react-native-web support, and they use expo-web for their example. If you feel strongly about this, I can revisit setting up a raw react-native-web instead of expo-web - but this will likely add at least a few hours of work.

I don't think requireNativeComponent should be removed. I think rn-web has a polyfill for that.

Hmm, I'll look into this, but I don't think this right. requireNativeComponent was definitely failing on web, with undefined is not a function. I'll look through their docs to see if I can find anything.

Also, try making use of the .web.tsx extension to override anything.

I'll look into this - but our use-case here doesn't 100% match what you'd typically use the .web/.native fork for. In this case, you're requiring native files, which don't work with the .web/.native pattern. Only for web is the module and "native" view going to be in JavaScript.

As for the TS error - make sure you're using the project's installed tsc instead of your global.

I'll look into this and see what's going on - honestly not sure which version I'm using.

I'm not very experienced with TypeScript, so I'm happy to take any recommendations for better typing.

mrousavy commented 3 years ago
  1. I see, I'll take a look at this and if it's really that difficult we'll just stick with expo for now.
  2. Yup, I browsed the rn-web source code and requireNativeComponent is actually not exported.
  3. You can just use .web.tsx to override the "normal" module. Example; Create a BlurhashModule.ts file which returns the result of requireNativeComponent, and BlurhashModule.web.ts which returns the web version of that. Everything in index.tsx operates the same.
  4. Yeah well everything is setup that you can just use VSCode with the ESLint extension and it will even automatically format everything on save. Make sure to select the TypeScript version in the command palette (I think in the latest insider they removed that)
jacob-israel-turner commented 3 years ago

@mrousavy I have a couple of TODO's left in the code here, but I'm pretty happy with where things are and I believe I've addressed your main concerns. Any other feedback or requests on this?

jacob-israel-turner commented 3 years ago

@mrousavy Sorry to be a squeaky wheel - I'm really excited to implement this in our app! 😂

Any chance you'd be able to look at this in the next couple days?

mrousavy commented 3 years ago

Hey! I understand, I also hate it when maintainers take too long to merge a perfect PR. Unfortunately I'm really really busy with the upcoming launch, but I'll promise I'll look into this sometime this week 🤞

Thanks for your awesome work!

mrousavy commented 3 years ago

Okay so before I review the code - there are a few things I don't like, can you tell me if there are alternative solutions for those?

  1. We now depend on the blurhash library. Now every single react-native user has to unnecessarily install that blurhash library from npm too (automatically, but still increases bundle size and download time for npm install)
  2. I don't want to include expo in the example. I'd rater use the bare react-native-web setup, ideally configured with ESBuild.

Do you think those are things that can be addressed?

jacob-israel-turner commented 3 years ago
  1. This is a good point. Probably doing a "blurry" dev dependency is the best option. I'll play around with this. This will require updating the install step of the docs to include blurhash if you want to use web.
  2. What is your hesitation with having expo in the example? I looked around at a few other web-compatible React Native libraries, and several use expo. It's super easy to setup and adds zero dependencies to the final product. I can look into doing a bare react-native-web setup again - but frankly if it takes more than a couple of hours to switch over it's probably not worth the time.
mrousavy commented 3 years ago

Ideally there should be no changes to the native libs, that includes no increased bundle size (which would be the case if we installed react-blurhash or just blurhash as a dependency) etc. That's not a problem with .web.tsx files as those get optimized away anyways. On the other hand I think I can use the blurhash lib for a few things I'm doing by hand in JS right now, I'll take a look at that when I get home.

My hesitation with expo is that it is a pretty big framework which is just installed to wrap a simple demo with react-native-web. It is definitely possible to only use rn-web and the official docs also show those steps. I received a PR for web support in my MMKV library which does not use expo, so maybe you can get some inspiration from there: https://github.com/mrousavy/react-native-mmkv/pull/45

jacob-israel-turner commented 3 years ago

Additional warning to handle, before merging: SharedArrayBuffer will require cross-origin isolation as of M91, around May 2021.

jacob-israel-turner commented 3 years ago

@mrousavy I'm a little confused by the build step, and distribution step here.

I'm finding locally, after building, a different result than what I get if I install from NPM. This is causing an issue in our web project because it isn't setup to use optional chaining (where react-native has supported it for a while).

I can go add support in our project if needed.

But I am confused, since the locally built file for react-native-blurhash under /lib/module/index.js looks like this:

Screen Shot 2021-04-09 at 4 42 44 PM

And the file downloaded from NPM under dist/index.js looks like this:

Screen Shot 2021-04-09 at 4 43 45 PM

Should the file published to NPM compile out the optional chaining syntax? Or should I update my project to compile it properly?

mrousavy commented 3 years ago

@jacob-israel-turner I'm sorry for the delay, I'm in the middle of launching our startup right now. I promise I'll look into this when I have some more free time 🤞

RWOverdijk commented 2 years ago

Did your startup succeed yet? 😄

mrousavy commented 2 years ago

Haha, no it did not. My new company is doing great though, and I think the PR needs to be rebased to be merged

iM-GeeKy commented 3 months ago

@jacob-israel-turner @mrousavy Is there any plans to get this feature into the project?

jacob-israel-turner commented 3 months ago

@iM-GeeKy @mrousavy I'm not going to put any more effort into this PR - I am no longer working on the project I was using blurhash for, and I am not currently working on any React Native or React Native Web projects. It's probably best to close this PR and open a new one, given branch drift.

mrousavy commented 3 months ago

@jacob-israel-turner Sorry for not reviewing/merging this back then. And still thank you very much for your efforts, I'm sure this will be helpful as a reference to implement web support at some point in the future 🙏