re-rxjs / react-rxjs

React bindings for RxJS
https://react-rxjs.org
MIT License
546 stars 18 forks source link

Fix compile error on Next.js 12 (Directory import) #239

Closed skve closed 2 years ago

skve commented 2 years ago

We're using react-rxjs but have transitioned away to an SSR framework, however Node doesn't seem to support this syntax of importing from a directory without specifically writing index.js.

Installing react-rxjs on a freshly setup Next.js project yields the following error when trying to import bind from @react-rxjs/core:

Error: Directory import '*<project>*/node_modules/use-sync-external-store/shim' is not supported resolving ES modules imported from *<project>*/node_modules/@react-rxjs/core/dist/core.es2019.mjs

Did you mean to import use-sync-external-store/shim/index.js?
skve commented 2 years ago

Once react 18 is out I suppose you can replace the shim with the actual useSyncExternalStore

josepot commented 2 years ago

Hi @skve ! Thanks a lot for reporting this and for opening a PR for fixing the issue.

I'm not completely convinced yet that this is the best fix for this issue. I mean, yes, it's quite possible that this is going to fix your issue, but I just want to make sure that's not going to break anything else.

The thing is that the React team has created this shim package so that library authors can start using use-sync-external-store before React 18 is published, which looks like a great idea b/c React is supposed to use the native React hook if available (i.e if the user has React >18) and if not, then it will fallback to the shim. However, there is an important inconvenient with the way that they've published that package, which is that it's only been published as CJS :upside_down_face: So, that makes things a bit complicated when trying to integrate it with a library like ours, b/c I certainly don't want to declare the shim as a peer-dependency...

Anyways, all this is to say that I just want to make sure that we look a bit deeper into this before we merge your PR. So, please give me a couple of days to look into this a bit further. In the meanwhile, if you want to help us, something that would be very helpful is if you could provide us with a basic project that reproduces the issue that you are running into. That would be greatly appreciated!

skve commented 2 years ago

Sure! https://github.com/skve/rxjs-demo-next

I simply ran yarn create next-app --typescript and then followed the docs introduction to add a demo. You'll get the same error I mentioned at compile time

josepot commented 2 years ago

@all-contributors please add @skve for bug and code

allcontributors[bot] commented 2 years ago

@josepot

I've put up a pull request to add @skve! :tada:

skve commented 2 years ago

Thanks a lot for this fix @skve ! Much much appreciated!

BTW there is a subtle bug with the demo that you provided us with and it took me a lot longer than what I'm willing to publicly admit to find the issue 🙈 (It should be export default TextInput; 🙂 )

Haha good spot, I'm surprised it compiled at all on my machine.