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.

Sign up for GitHub

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

version on react + run prettier #537

Merged
merged 3 commits into from
Aug 18, 2023
Merged

version on react + run prettier #537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
295c70e
version on react + run prettier
Aug 17, 2023
a71d27e
fix specifier
Aug 17, 2023
993b534
react-dom/client
Aug 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 22 additions & 7 deletions packages/kitchen-sink/src/main.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,19 @@
import { StrictMode, useState, type ComponentType } from 'react';
import { createRoot } from 'react-dom/client';
import { version, StrictMode, useState, type ComponentType } from 'react';
import './style.css';
import { ErrorBoundary } from 'react-error-boundary';

type Module = Record<string, ComponentType>;

(async () => {
let createRootElement;
if (version.startsWith('18')) {
// eslint-disable-next-line @typescript-eslint/no-var-requires
const { createRoot } = await import('react-dom/client');
createRootElement = createRoot;
} else {
const ReactDOM = await import('react-dom');
createRootElement = ReactDOM.render;
}
const modules = await Promise.all(
Object.entries(import.meta.glob('./examples/*.tsx')).map(
async ([key, mod]) =>
Expand Down Expand Up @@ -85,9 +93,16 @@ type Module = Record<string, ComponentType>;
);
}

createRoot(document.getElementById('root')!).render(
<StrictMode>
<App />
</StrictMode>,
);
version.startsWith('18')
? createRootElement(document.getElementById('root')!).render(
<StrictMode>
<App />
</StrictMode>,
)
: createRootElement(
<StrictMode>
<App />
</StrictMode>,
document.getElementById('root')!,
);
})();
27 changes: 19 additions & 8 deletions packages/react/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import { Fragment, createElement, isValidElement } from 'react';
import { createRoot } from 'react-dom/client';
import { Fragment, createElement, isValidElement, version } from 'react';
import { REACT_ROOT, REGISTRY, RENDER_SCOPE } from './constants';
import type { ComponentProps, ReactNode, Ref } from 'react';
import type { Root } from 'react-dom/client';
import type { VNode } from '../million';

// TODO: access perf impact of this
Expand Down Expand Up @@ -45,12 +43,25 @@ export const renderReactScope = (vnode: ReactNode, unstable?: boolean) => {
}

const scope = (el: HTMLElement | null) => {
let root;
const parent = el ?? document.createElement(RENDER_SCOPE);
const root =
REACT_ROOT in parent
? (parent[REACT_ROOT] as Root)
: (parent[REACT_ROOT] = createRoot(parent));
root.render(vnode);
if (version.startsWith('18')) {
import('react-dom/client')
Copy link
Contributor

Choose a reason for hiding this comment

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

This might break Webpack which will attempt to import react-dom/client even if using react 17 because bundlers are not smart enough to evaluate version.startsWith('18').

I tested with and got the following error on https://github.com/getsentry/sentry (which uses React 17).

ERROR in ../node_modules/million/dist/chunks/utils.mjs 46:6-32
Module not found: Error: Can't resolve 'react-dom/client' in '/Users/abhijeetprasad/workspace/sentry/node_modules/million/dist/chunks'
resolve 'react-dom/client' in '/Users/abhijeetprasad/workspace/sentry/node_modules/million/dist/chunks'
  Parsed request is a module
  using description file: /Users/abhijeetprasad/workspace/sentry/node_modules/million/package.json (relative path: ./dist/chunks)

IMO the more holistic solution is to actually not do this built time - but instead split up the renderReactScope function into renderReactScopev17andbelow and renderReactScope and inject different one's depending on what version of react you're using, so we take care of this at built time instead of run time.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also tried adding million to https://github.com/romansndlr/react-vite-realworld-example-app with this patch (vite app using React 17) and things broke during build as well.

[vite]: Rollup failed to resolve import "react-dom/client" from ".yalc/million/dist/chunks/utils.mjs".
This is most likely unintended because it can break your application at runtime.
If you do want to externalize this module explicitly add it to
`build.rollupOptions.external`
error during build:
Error: [vite]: Rollup failed to resolve import "react-dom/client" from ".yalc/million/dist/chunks/utils.mjs".
This is most likely unintended because it can break your application at runtime.

.yalc comes from https://github.com/wclr/yalc - which is what I used to test million locally.

Copy link
Contributor Author

@rishi-raj-jain rishi-raj-jain Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think million new version containing this fix is yet to be released. Can you try replacing the node_modules/million/dist folder with the node_modules/million/dist folder from this PR's build?

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worthwhile to also test Next 12 - which many folks are still using because they can't upgrade to Next 13 yet, and Next 12 uses React 17 by default.

Next.js and it's webpack setup might also cause issues here.

Copy link
Contributor

@AbhiPrasad AbhiPrasad Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think million new version containing this fix is yet to be released. Can you try replacing the dist folder with the dist folder from this?

I'm using the most recent version on master branch! I built the repo locally and directly imported it into my project via yalc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, appreciate the feedback and you reaching out!

.then((res) => {
root =
REACT_ROOT in parent
? parent[REACT_ROOT]
: (parent[REACT_ROOT] = res.createRoot(parent));
root.render(vnode);
})
.catch((e) => {
// eslint-disable-next-line no-console
console.error(e);
});
} else {
root = parent[REACT_ROOT];
root.render(vnode);
}
return parent;
};

Expand Down
75 changes: 46 additions & 29 deletions website/components/million-library/react.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,52 +5,66 @@ Object.defineProperty(exports, '__esModule', { value: true });
const react = require('react');
const block$1 = require('./chunks/block.cjs');
const constants = require('./chunks/constants.cjs');
const client = require('react-dom/client');

const REACT_ROOT = "__react_root";
const RENDER_SCOPE = "million-render-scope";
const REACT_ROOT = '__react_root';
const RENDER_SCOPE = 'million-render-scope';
const renderReactScope = (jsx) => {
return (el) => {
let root;
const parent = el ?? block$1.document$.createElement(RENDER_SCOPE);
const root = REACT_ROOT in parent ? parent[REACT_ROOT] : parent[REACT_ROOT] = client.createRoot(parent);
if (react.version.startsWith('18')) {
// eslint-disable-next-line @typescript-eslint/no-var-requires
const { createRoot } = require('react-dom/client');
root =
REACT_ROOT in parent
? parent[REACT_ROOT]
: (parent[REACT_ROOT] = createRoot(parent));
} else {
root = parent[REACT_ROOT];
}
root.render(jsx);
return parent;
};
};
const unwrap = (vnode) => {
if (typeof vnode !== "object" || vnode === null || !("type" in vnode)) {
if (typeof vnode === "number" || vnode === true) {
if (typeof vnode !== 'object' || vnode === null || !('type' in vnode)) {
if (typeof vnode === 'number' || vnode === true) {
return String(vnode);
} else if (!vnode) {
return void 0;
}
return vnode;
}
const type = vnode.type;
if (typeof type === "function") {
if (typeof type === 'function') {
return unwrap(type(vnode.props ?? {}));
}
if (typeof type === "object" && "$" in type)
return type;
if (typeof type === 'object' && '$' in type) return type;
const props = { ...vnode.props };
const children = vnode.props?.children;
if (children !== void 0 && children !== null) {
props.children = flatten(vnode.props.children).map(
(child) => unwrap(child)
props.children = flatten(vnode.props.children).map((child) =>
unwrap(child),
);
}
return {
type,
props
props,
};
};
const flatten = (rawChildren) => {
if (rawChildren === void 0 || rawChildren === null)
return [];
if (typeof rawChildren === "object" && "type" in rawChildren && rawChildren.type === react.Fragment) {
if (rawChildren === void 0 || rawChildren === null) return [];
if (
typeof rawChildren === 'object' &&
'type' in rawChildren &&
rawChildren.type === react.Fragment
) {
return flatten(rawChildren.props.children);
}
if (!Array.isArray(rawChildren) || typeof rawChildren === "object" && "$" in rawChildren) {
if (
!Array.isArray(rawChildren) ||
(typeof rawChildren === 'object' && '$' in rawChildren)
) {
return [rawChildren];
}
const flattenedChildren = rawChildren.flat(Infinity);
Expand All @@ -67,14 +81,16 @@ if (CSSStyleSheet.prototype.replaceSync) {
sheet.replaceSync(css);
document.adoptedStyleSheets = [sheet];
} else {
const style = document.createElement("style");
const style = document.createElement('style');
document.head.appendChild(style);
style.type = "text/css";
style.type = 'text/css';
style.appendChild(document.createTextNode(css));
}
const REGISTRY = new constants.Map$();
const block = (fn, options = {}) => {
const block2 = constants.MapHas$.call(REGISTRY, fn) ? constants.MapGet$.call(REGISTRY, fn) : block$1.block(fn, unwrap);
const block2 = constants.MapHas$.call(REGISTRY, fn)
? constants.MapGet$.call(REGISTRY, fn)
: block$1.block(fn, unwrap);
function MillionBlock(props) {
const ref = react.useRef(null);
const patch = react.useRef(null);
Expand All @@ -98,7 +114,7 @@ const block = (fn, options = {}) => {
react.Fragment,
null,
marker,
react.createElement(Effect, { effect })
react.createElement(Effect, { effect }),
);
return vnode;
}
Expand All @@ -117,24 +133,25 @@ const MillionArray = ({ each, children }) => {
const fragmentRef = react.useRef(null);
const cache = react.useRef({
each: null,
children: null
children: null,
});
if (fragmentRef.current && each !== cache.current.each) {
const newChildren = createChildren(each, children, cache);
block$1.arrayPatch$.call(fragmentRef.current, block$1.mapArray(newChildren));
block$1.arrayPatch$.call(
fragmentRef.current,
block$1.mapArray(newChildren),
);
}
react.useEffect(() => {
if (fragmentRef.current)
return;
if (fragmentRef.current) return;
const newChildren = createChildren(each, children, cache);
fragmentRef.current = block$1.mapArray(newChildren);
block$1.arrayMount$.call(fragmentRef.current, ref.current);
}, []);
return react.createElement(RENDER_SCOPE, { ref });
};
const For = react.memo(
MillionArray,
(oldProps, newProps) => Object.is(newProps.each, oldProps.each)
const For = react.memo(MillionArray, (oldProps, newProps) =>
Object.is(newProps.each, oldProps.each),
);
const createChildren = (each, getComponent, cache) => {
const children = Array(each.length);
Expand All @@ -153,13 +170,13 @@ const createChildren = (each, getComponent, cache) => {
const block = block$1.block((props) => {
return {
type: RENDER_SCOPE,
props: { children: [props?.__scope] }
props: { children: [props?.__scope] },
};
});
const currentBlock = (props) => {
return block({
props,
__scope: renderReactScope(react.createElement(vnode.type, props))
__scope: renderReactScope(react.createElement(vnode.type, props)),
});
};
constants.MapSet$.call(REGISTRY, vnode.type, currentBlock);
Expand Down
Loading