-
-
Notifications
You must be signed in to change notification settings - Fork 75
WIP|Implement for react-native-web #112
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
Conversation
Allows us to add typedef and import implementation from different locations based on plaform
This reverts commit dbb5e91.
@@ -1,9 +1,14 @@ | |||
require_relative '../node_modules/react-native/scripts/react_native_pods' | |||
require_relative '../node_modules/@react-native-community/cli-platform-ios/native_modules' | |||
|
|||
use_modular_headers! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could not get the current iOS example to run without these changes.
src/blurhashModule.ts
Outdated
@@ -0,0 +1,12 @@ | |||
import { NativeModules, Platform } from 'react-native'; | |||
const BlurhashModule: BlurhashModule = Platform.select({ | |||
web: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
src/index.tsx
Outdated
@@ -123,7 +122,10 @@ export class Blurhash extends React.PureComponent<BlurhashProps> { | |||
if (this.props.onLoadEnd != null) this.props.onLoadEnd(); | |||
} | |||
_onLoadError(event?: NativeSyntheticEvent<{ message?: string }>) { | |||
if (this.props.onLoadError != null) this.props.onLoadError(event?.nativeEvent?.message); | |||
if (this.props.onLoadError != null) { | |||
const message = event?.nativeEvent?.message; // TODO: Not sure how to get proper value here on web |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
src/index.tsx
Outdated
@@ -132,16 +134,14 @@ export class Blurhash extends React.PureComponent<BlurhashProps> { | |||
{...this.props} | |||
onLoadStart={this._onLoadStart} | |||
onLoadEnd={this._onLoadEnd} | |||
// @ts-expect-error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was giving me a build error
src/index.tsx
Outdated
// @ts-expect-error this second argument is still not public, but probably required for TurboModules. | ||
Blurhash, | ||
); | ||
const NativeBlurhashView = Platform.OS === 'web' ? require('./web/blurhashView').default : requireNativeComponent<BlurhashProps>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried a couple iterations of this.
Setting NativeBlurhashView
as a let
then assigning within an if statement, caused issues with Typescript. Didn't like that it was unset for a short period.
Using Platform.select
still caused reuireNativeComponent
to be called on web. In react-native
, irrelevant Platform.select
branches are removed at build time - doesn't look like that happens with react-native-web
, unfortunately.
src/web/blurhashView.tsx
Outdated
export default function BlurhashView ({ | ||
blurhash, | ||
decodeHeight = 128, | ||
decodePunch, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to test and ensure punch is implemented properly.
src/web/canvas.tsx
Outdated
}; | ||
|
||
export default function BlurhashCanvas ({ | ||
// TODO: See how canvas handles unset width/height |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// TODO: See how canvas handles unset width/height |
I'd also like to cleanup the commit history before this merges. |
@mrousavy looks like the linting step failed because |
Received the following from @mrousavy via email, figured it would make sense to reply here:
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
Hmm, I'll look into this, but I don't think this right.
I'll look into this - but our use-case here doesn't 100% match what you'd typically use the
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. |
|
src/blurhashModule.web.ts
Outdated
const loadImage = (src: string): Promise<HTMLImageElement> => | ||
new Promise((resolve, reject) => { | ||
const img = document.createElement('img'); | ||
img.crossOrigin = 'Anonymous'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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? |
@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? |
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! |
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?
Do you think those are things that can be addressed? |
|
Ideally there should be no changes to the native libs, that includes no increased bundle size (which would be the case if we installed My hesitation with expo is that it is a pretty big framework which is just installed to wrap a simple demo with |
Additional warning to handle, before merging: |
@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 And the file downloaded from NPM under Should the file published to NPM compile out the optional chaining syntax? Or should I update my project to compile it properly? |
This reverts commit 5dbb7af.
@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 🤞 |
Did your startup succeed yet? 😄 |
Haha, no it did not. My new company is doing great though, and I think the PR needs to be rebased to be merged |
@jacob-israel-turner @mrousavy Is there any plans to get this feature into the project? |
@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 🙏 |
No description provided.