Closed mcgear closed 10 months ago
When I upgraded to Fresh 1.4, I am finding that children get set to undefined when rendered. I moved the same value to a property and everything worked as before.
@kitsonk do you have a link to a GitHub repo where the problem can be reproduced?
This commit of the site is what should be able to reproduce it: https://github.com/o-biotech/biotech-manager-web/blob/7d075f0b62c001d219fad0d8233fe3e71b0e19e6/
Specifically in the components/organisms/BiotechHeader.tsx
@marvinhagemeister this is what I had to do to fix it as part of the upgrade: https://github.com/ts-why/tswhy/commit/5eb95c84311e7d232d764fdbe7d76e052572f95a#diff-40d9bbe97e321d3a88ebe909c1bb3a7b3f2b383aa00dcd5192e28a2fb4a1b67e
I will try to later on to create a branch with that change backed out.
@marvinhagemeister this branch replicates the issue: https://github.com/ts-why/tswhy/tree/children_islands
If you clone, deno task start
and navigate to: http://localhost:8000/edit?code=1002 and look at the console, you see the island is complaining about an object being undefined. If you move the data to a property (revert the last commit in that branch: https://github.com/ts-why/tswhy/commit/6bfb6c9a813a8e9d32ecbe082177823a088ccade) it will work.
I never updated this code base to 1.3 so I am not sure if it was introduced there or 1.4.
Thanks for the branch @kitsonk. When I run your code I get:
diagnostics.ts:38 Uncaught (in promise) TypeError: Cannot destructure property 'title' of 'object null' as it is null.
at diagnosticDataToMarkdown (diagnostics.ts:38:5)
at T.Editor [as constructor] (Editor.tsx:42:14)
at T.ce [as render] (preact.mjs:2:8524)
at O (preact.mjs:2:6232)
at X (preact.mjs:2:2148)
at O (preact.mjs:2:6446)
at fe (preact.mjs:2:8639)
at _render (main.ts:258:15)
I've created an even more minimal reproduction here. When I run this, I get:
childrenProps.tsx:18 Uncaught (in promise) TypeError: Cannot destructure property 'title' of 'object null' as it is null.
at helper (childrenProps.tsx:18:26)
at T.Bug [as constructor] (Bug.tsx:4:16)
at T.ce [as render] (preact.mjs:2:8524)
at O (preact.mjs:2:6232)
at X (preact.mjs:2:2148)
at O (preact.mjs:2:6446)
at fe (preact.mjs:2:8639)
at _render (main.ts:258:15)
So I think it's the exact same case. I've just cut out as much code as possible to get to the issue.
If I have time I'll continue debugging, but I just wanted to share this here as a starting point for anyone else.
Yes, it appears that for whatever reason when the island is hydrated on the client side, it's children value is undefined
, but when it is passed as a prop, it hydrates fine. Now theory could be that fresh is only dealing with vdom nodes for children elements when hydrating (though previous versions seemed to deal with it fine).
I think i've created a similar issue in how i am trying to use our new atomic_icons library in a basic fresh starter. I have created a repo that contains a reproduction of the issue here: https://github.com/mcgear/deno-fresh-icons-in-island
To setup the repo, used the following command to setup fresh (make sure the folder is empty): npx fathym dev deno scaffold
Choose the fresh
option
Once that is setup, you have to add the atomic-icons library as shown in the docs for the fresh plugin here: https://deno.land/x/fathym_atomic_icons@v0.0.3-integration21#getting-started
Then you need to configure the plugin as shown: https://deno.land/x/fathym_atomic_icons@v0.0.3-integration21#automatic-configuration-with-fresh
Now: deno task start
The issue arises in that the Counter buttons no longer work and the console throws an error: Uncaught ReferenceError: Deno is not defined at build_id.ts:3:22 (anonymous) @ build_id.ts:3
If you go into the Counter.tsx file and comment out the ExclaimIcon at line 14, everything goes back to working just fine.
We have some other controls that seem to maybe be impacted by similar issues.
@mcgear Sorry for the late response. I was away on vacation and I'm back now. I'm currently looking into this, but I'm unable to run the reproduction case. It seems like something is wrong with the icons. Things I tried:
deno task start
after fresh clone of the repo gives me this
error: Uncaught (in promise) TypeError: Module not found "file:///Users/marvinh/dev/test/biotech-manager-web/build/iconset/icons/_exports.ts".
at file:///Users/marvinh/dev/test/biotech-manager-web/routes/index.tsx:13:8
await import(entrypoint);
^
at async dev (https://deno.land/x/fresh@1.4.2/src/dev/dev_command.ts:40:5)
at async file:///Users/marvinh/dev/test/biotech-manager-web/dev.ts:5:1
icons
task first, so I did that, but that gives me this error:
Task icons deno run -A ./scripts/icons.atomic.ts
error: Module not found "file:///Users/marvinh/dev/test/biotech-manager-web/iconset.config.ts".
at file:///Users/marvinh/dev/test/biotech-manager-web/scripts/icons.atomic.ts:2:34
fathym-atomic-icons.config.ts
is supposed to be the config, so I copied the contents to iconset.config.ts
. But this lead to this error
error: Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'IconMap')
const map = Object.keys(this.config.IconMap).map(
^
at IconSet.ToSheet (https://deno.land/x/fathym_atomic_icons@v0.0.3-integration1/src/sprites/IconSet.tsx:194:41)
at useIconSet (https://deno.land/x/fathym_atomic_icons@v0.0.3-integration1/src/sprites/IconSet.tsx:19:33)
at useFileIconSet (https://deno.land/x/fathym_atomic_icons@v0.0.3-integration1/src/sprites/IconSet.tsx:28:9)
at file:///Users/marvinh/dev/test/biotech-manager-web/scripts/icons.atomic.ts:4:7
at eventLoopTick (ext:core/01_core.js:183:11)
What do I need to do to get the reproduction case to run?
@kitsonk passing JSON as children
in JSX for islands is not supported. The children
prop for islands must be a JSX element. For serializing non-JSX data passing it as a prop is the way to go. Whilst it technically works in Preact to pass random data as children
as long as the receiving component can work with that, it's a bit of an anti-pattern since it breaks the compositional nature of components.
Regardless, it's something where we should show a nice error message instead of silently failing. Made https://github.com/denoland/fresh/pull/1736 to address that.
@marvinhagemeister I just re-cloned and did not run into the issue that you had, everything just worked.
Then i noticed that you were trying the repro from my initial post, which would require cloning that specific commit. I made a simpler repro that seems to be working if you try it: https://github.com/mcgear/deno-fresh-icons-in-island
Typically we don't checkin the build directory for our projects, and force the icons to be generated the first time. The issue we have here on first build, is that if you are using the icons the build fails, and the generator code never fully executes.
In the latest repro (https://github.com/mcgear/deno-fresh-icons-in-island) i just checked in the build dir to ensure you didn't run into this issue.
I have also updated the fathym-deno/atomic-icons code to have a www folder that replicates the issue and shows the Deno not defined error:
Just clone:
And then cd www
and finally deno task start
This is the error we see
build_id.ts:3 Uncaught ReferenceError: Deno is not defined
at build_id.ts:3:22
@mcgear thanks for providing a repro. With that I was able to reproduce the issue on my end.
The build errors in https://github.com/mcgear/deno-fresh-icons-in-island are caused by importing the src.deps.ts
file. That file imports a bunch of dependencies that cannot be bundled for the browser. svgo
for example requires node's os
module which doesn't exist in the browser. Same is true for https://github.com/fathym-deno/atomic-icons . There is a src.deps.ts
file as well which contains imports that cannot run in the browser. This includes Fresh's asset()
function which currently cannot run in the browser either. But now seeing that I'm kinda wondering if we should make it work in the browser.
In either way, we should print a better error message when server-only code is being loaded in an island in Fresh. The real issue though is that the Deno registry has no tooling or guardrails in place to catch that during publishing. I'll forward that issue to the relevant folks working on the registry as this is definitely a frustrating experience. We should do better here.
The workaround for now is to have two separate deps.ts
files: One for server stuff, and one that imports the dependencies for the Preact components. It might make sense to have an additional top level entry browser.ts
next to mod.ts
and then import only that in your Fresh islands.
awesome @marvinhagemeister, thank you for the help on this. That all makes sense.
I should have also mentioned a +1 for asset()
working in the browser too
Closing this as the original error has been resolved.
I am trying to figure out how to support this, and will try to setup the scenario.
We have a shared component library with some of our companies reusable deno building blocks. Specifically in there, we have a control to help show items or hide them behind a popup based on screen size. We call this our ResponsiveSet: https://github.com/fathym-deno/atomic/blob/integration/src/molecules/ResponsiveSet.tsx
Its heavy lifting is done by the MenuButton control which hides its children behind a button click: https://github.com/fathym-deno/atomic/blob/integration/src/molecules/MenuButton.tsx
You'll see the
{props.toggleChildren}
is rendered inside an Action to wire up the onclick to show the menu.In order to make this control usable we have to wrap the ResponsiveSet in an island component to make the MenuButton interactivity work. We do this in our Fresh project with the InteractiveResponsiveSet: https://github.com/o-biotech/biotech-manager-web/blob/integration/islands/molecules/InteractiveResponsiveSet.tsx
This works and the whole control works, the only issue is that the toggleChildren won't render the icon we are trying to get inside the action: https://github.com/o-biotech/biotech-manager-web/blob/7d075f0b62c001d219fad0d8233fe3e71b0e19e6/components/organisms/BiotechHeader.tsx
There seems to be something going on when registering it with the InteractiveResponsiveSet, if you take the BiotechHeader and instead use the ResponsiveSet itself, the MenuIcon renders (but interactivity is lost).
In our latest integration branch, you can see that if we set the triggerChildren to a simple
toggleChildren="☰"
value, it is passed through just fine: https://github.com/o-biotech/biotech-manager-web/blob/integration/components/organisms/BiotechHeader.tsxDoes this have to do with issues in passing props between server and island components? Is there a better way i should be registering Islands in the component library (i know i'd like a better way than the wrapper).
Thanks for the help, let me know how else i can help.