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

feat: Add static build mechanism #1440

Merged

Conversation

lucas-varela
Copy link
Collaborator

No description provided.

@lucas-varela lucas-varela requested a review from wodenx March 9, 2022 16:23
return WithoutHydration;
};

export const withoutHydration: WithoutHydrationFunction = (options) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add some doc blocks?

@@ -0,0 +1,6 @@
#!/usr/bin/env node
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is extraneous.

@@ -0,0 +1,67 @@
# Bodiless-JS CLI
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also extraneous.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but we should add an accurate readme describing how to use the static thing. I have one in progress - i will issue apr.

@@ -0,0 +1,33 @@
{
"name": "@bodiless/hydration",
"version": "0.3.7",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll hve to update the version now that there has been a release.

@cypress
Copy link

cypress bot commented Mar 11, 2022



Test summary

64 0 0 0Flakiness 0


Run details

Project bodiless
Status Passed
Commit 3eb87a2 ℹ️
Started Mar 21, 2022 12:35 PM
Ended Mar 21, 2022 12:43 PM
Duration 07:46 💡
OS Linux Ubuntu - 20.04
Browser Chrome 98

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

const id = `${path.join('-')}-${nodeKey}`;

return (
<WrapperElement data-no-hydrate id={id}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcopagliarulo should we use a different attribute here or even just mke this the value of the data-no-hydrate ttribute? i'm a bit worried about using id.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is required because then the element is retrieved using document.getElementById. Using the attribute data-no-hydrate means that we should traverse the whole tree to retrieve the element. What's your concern about using id?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my concern is that id's must be unique, and we are using one here without concern for what other code might do to the dom. Couldn't we just use document.querySelector? Or are you concerned that will be significantly less performant? Anyway, I guess this is not a critical concern.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually querySelector is faster than getElementById, I am fine to use data attribute to store the Id and then querySelector

const [shouldHydrate, setShouldHydrate] = useState<boolean | undefined>(undefined);

const { path } = useNode().node;
const { nodeKey = ''} = {...props};
const id = `${path.join('-')}-${nodeKey}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be better if the logic that sets the id were encpsulated rather than repeated

@@ -16,7 +16,8 @@ import { HOC } from '@bodiless/fclasses';

type WithoutHydrationOptions = {
onUpdate?: (props: Record<string, any>, element: HTMLElement | null) => void
disableFallback?: boolean
disableFallback?: boolean,
WrapperElement?: 'div'|'span',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcopagliarulo and @lucas-varela are we happy with limiting this to div or span, or should any element be allowed?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine. It will prevent issues with dangerouslySetInnerHTML that doesn't allow to set inner html containing unallowed tags, e.g. set p tag withing a p wrapper.

@@ -61,27 +79,80 @@ const withoutHydrationClientSide: WithoutHydrationFunction = ({
onUpdate(props, rootRef.current);
});

useLayoutEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to have a comment here explaining what we are doing...

wodenx
wodenx previously approved these changes Mar 20, 2022
wodenx
wodenx previously approved these changes Mar 21, 2022
@lucas-varela lucas-varela merged commit 22384f1 into johnsonandjohnson:main Mar 21, 2022
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.

None yet

5 participants