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

Drop cljsjs react in favour of node modules #16

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

danieroux
Copy link

@danieroux danieroux commented Feb 22, 2023

Change the requires to pull in the node_modules installed react.

This change is needed for: awkay/workspaces#6

@awkay
Copy link
Member

awkay commented Feb 22, 2023

Technically I don't see any actual need to require them at all now. Neither of those namespaces uses React directly. It used to be that you needed to make sure the global js/React and such were defined, but nowadays things work a lot better. This is some old code. You might glance through the rest of the source and see if there are any js/React* usages, because that would also be bad.

@awkay
Copy link
Member

awkay commented Feb 22, 2023

Yeah, I looked. I don't thing any requires for React are needed. Fulcro's namespaces take care of the low-level react stuff.

@danieroux
Copy link
Author

This call in Fulcro is a challenge.

Since requiring ["react-dom/server" :as react.server] in that ns will declare a hard dependency on react-dom-server that Fulcro would not want?

The tests for fulcro-garden-css fails because of this (and also, the tests in this PR is failing already. Ugh.)

The tests pass if I invoke (react.server/renderToString c) instead.

@awkay
Copy link
Member

awkay commented Feb 23, 2023

So, yes, if there are any direct refs to react things in this lib, then they need to be resolved as you noted. Yes, Fulcro's not going to change to service this lib. This lib should be fixed to do things correctly, including the tests.

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

Successfully merging this pull request may close these issues.

2 participants