RFC: Privatize some options (-7 B) by andrewiggins · Pull Request #1692 · preactjs/preact

andrewiggins · 2019-06-08T06:49:33Z

Our options hooks are an important and useful part of our library that serve many purposes. For examples, see Marvin's blog or #1690.

However, given our current bundle size (~3.4k) many changes going forward will likely need to remove or replace code in order to remain small and stay within our size budget (e.g. #1688). In other words, PRs may need to refactor internals in order to stay small. Exposing too many internal details of how our diff works makes these kind of refactors more difficult and could prevent us from making the right changes to stay within our size budget.

My concern is that our current set of options may expose publicly too many internal details and could impact our ability to keep our size down in the future. While working on #1688 I had to make a breaking change to our options. While it's possible that the specific issue in #1688 could be resolved differently, it made me worried that for other changes, we wouldn't be able to adjust our internals to reduce the impact of the PR. Of course, while all libraries need to abide by the public API they ship and not break it unnecessarily, I'd like to reduce our current public API surface to enable more kinds of changes than the current API allows - specifically privatize some of our options.

Again, I really think our options are an important part of public API, and for all the options I suggest removing, I think we can bring them back in the future. But I think I'd like to release Preact X with a minimal set, and only make more options public once some new features have stabilized more (e.g. Suspense) and we have compelling user scenario(s) an option lights up.

Note: this isn't a comment on internal usage of options. We can add/remove as we please since they are all internal and served from the same package.

Thoughts?

Cheers,
Andre

P.S. Below in index.d.ts I've left comments under each option I'm proposing to privatize. If you have thoughts about a specific option, leave it in the thread there.

coveralls · 2019-06-08T06:52:51Z

Coverage remained the same at 99.778% when pulling c2554e1 on privatize-options into 93c626e on master.

andrewiggins

I believe the options that remain serve as a reasonable back-compat story for the options from Preact 8. Let me know what you think!

andrewiggins · 2019-06-08T07:08:41Z

 	 */
 	interface Options {
-		/** Attach a hook that is invoked before render, mainly to check the arguments. */
-		root?(vnode: ComponentChild, parent: Element | Document | ShadowRoot | DocumentFragment): void;


I think in user-land, most things you can do with root could be accomplished by just wrapping render. I think this pattern of composing functions (e.g. let render = wrapper(render)) is a much more powerful pattern that can do what this hook does and more, and so I think I'd encourage our users to do that over use this root hook.

Agree, it's mainly needed for devtools and I doubt there is much use beyond that 👍

andrewiggins · 2019-06-08T07:09:42Z

-		/** Attach a hook that is invoked before render, mainly to check the arguments. */
-		root?(vnode: ComponentChild, parent: Element | Document | ShadowRoot | DocumentFragment): void;
 		/** Attach a hook that is invoked whenever a VNode is created. */
-		vnode(vnode: VNode): void;


Note: I just fixed the vnode type to specify that this is optional (i.e. vnode?()). I'm not suggesting we remove this hook. It's arguably our most powerful!

andrewiggins · 2019-06-08T07:14:06Z

 		/** Attach a hook that is invoked whenever a VNode is created. */
-		vnode(vnode: VNode): void;
-		/** Attach a hook that is invoked after a tree was mounted or was updated. */
-		commit?(vnode: VNode): void;


I think our idea of a commit phase is pretty new and not fully fleshed out yet. I think I'd like to hold back on what a commit hook looks like until we have explored (or decided not to explore) a fully two-phased render. Depending on how we do that, this hook may look very different!

Good call! It will likely change a lot if we'll work on 2-phase commit 👍

andrewiggins · 2019-06-08T07:19:51Z

 		/** Attach a hook that is invoked before a vnode is diffed. */
 		diff?(vnode: VNode): void;
-		/** Attach a hook that is invoked before a vnode has rendered. */
-		render?(vnode: VNode): void;


Could this be accomplished with a vnode hook that wraps a component's render method, similar to the wrapping I referred to in root? Wrapping the render method would probably be way more useful cuz that way the wrapper gets access to the component (through this) and the vnode (as the parameter to the vnode hook). Currently a public render option wouldn't be able to access the component of the VNode cuz it's a private property on the VNode.

I like this idea, never thought about it that way 👍 That would be a lot more powerful and would likely be easier to understand for users 👍

andrewiggins · 2019-06-08T07:24:39Z

-		/** Attach a hook that is invoked before a vnode has rendered. */
-		render?(vnode: VNode): void;
-		/** Attach a hook that is invoked before a hook's state is queried. */
-		hook?(component: Component): void;


Hooks are pretty new and there might still be some bytes we can squeeze out of them yet! This is one option that we could perhaps leave in if someone feels strongly, but absent of that, I still think I'd prefer to not expose it by default.

Yup, we can remove it. A hook like this or something else will be needed for the devtools in the future. It needs some way to know which hook was called. But again this can be postponed and I agree with your general sentiment that we should only expose them if they are really needed.

andrewiggins · 2019-06-08T07:33:57Z

 		/** Attach a hook that is invoked after a vnode has rendered. */
 		diffed?(vnode: VNode): void;
-		/** Attach a hook that is invoked after an error is caught in a component but before calling lifecycle hooks */
-		catchError?(error: any, component: Component): void;


I have a couple issues with the catchError and catchRender options:

These two options expose that we have multiple try/catch's in our codebase. Will that always be true? I'd personally like to reduce that number to save bytes but having to support both these options may make that difficult/less useful.

The component passed in to the option is the first parent of the throwing component. Is that the right thing to pass in here? Or would user's want/expect to be given the component that threw? Without any good user scenarios, it's hard to know what to do here.

The _ancestorComponent pointer (soon to be _parent) is a private field and not accessible to users, which I think reduces the value of a catch* hook. Since we currently pass in the parent component (which may or may not be an error boundary), I'm uncertain what a good use case for this option is.

+1 On making it private

andrewiggins · 2019-06-08T07:37:08Z

-		 *
-		 * @return Return a boolean indicating whether the error was handled by the hook or not
-		 */
-		catchRender?(error: any, component: Component): boolean;


See comment for catchError

andrewiggins · 2019-06-08T07:39:45Z

+		/** Attach a hook that is invoked immediately before a vnode is unmounted. */
 		unmount?(vnode: VNode): void;
 		/** Attach a hook that is invoked before a vnode is diffed. */
 		diff?(vnode: VNode): void;


I'm a little torn on this one. I'm not sure how it could be used, but it feels complimentary to diffed. Perhaps we should leave it out until we have a reason not to? Not sure...

Yeah, this hook is tough. Wondering if most of it could be accomplished by mutating a vnode in options.vnode() instead. This one is mainly needed for profiling purposes as we need to have a marker when the node actually starts and finishes rendering.

I went ahead and marked it as private. If we want to bring it back it's as simple as reverting the latest commit, so just let me know.

andrewiggins · 2019-06-08T07:41:49Z

-		 */
-		catchRender?(error: any, component: Component): boolean;
 		event?(e: Event): void;
+		requestAnimationFrame?: typeof requestAnimationFrame;


I figured it made sense to leave this one as public since it is a core scheduling primitive of useEffect hooks, similar to how debounceRendering is the scheduling primitive for state updates

marvinhagemeister

This is a very good direction we're moving in with this PR 👍 Only exposing options that are actually needed makes it a lot safer to explore with various implementations of our internals 💯 Outstanding work as usual Andre 🥇

robertknight · 2019-06-08T21:18:17Z

I'm in agreement with the broad principles outlined in the PR description. While Preact 10's internals are still new and evolving, I think it makes sense to be conservative with the options hooks that are exported. Also, if someone does have a need for a private hook, that will hopefully lead them to file an issue and give us useful information about their use case.

andrewiggins force-pushed the privatize-options branch 2 times, most recently from e35ca76 to 1645fba Compare June 8, 2019 07:05

andrewiggins mentioned this pull request Jun 8, 2019

Replace ancestorComponent with vnode parent pointer (+5 B) #1688

Merged

andrewiggins commented Jun 8, 2019

View reviewed changes

Privatize some options (-7 B)

1d8d60f

andrewiggins force-pushed the privatize-options branch from 1645fba to 1d8d60f Compare June 8, 2019 07:48

marvinhagemeister approved these changes Jun 8, 2019

View reviewed changes

andrewiggins and others added 4 commits June 9, 2019 20:59

Privatize diff option

c477c43

Merge branch 'master' into privatize-options

618b3f0

Fix up catch* options in internal.d.ts

c79b008

Merge branch 'master' into privatize-options

c2554e1

marvinhagemeister merged commit 1dad35c into master Jun 10, 2019

marvinhagemeister deleted the privatize-options branch June 10, 2019 09:20

snyk-bot mentioned this pull request Oct 26, 2019

[Snyk] Upgrade preact from 8.5.2 to 10.0.1 ajesse11x/now-examples#6

Open

Uh oh!

Conversation

andrewiggins commented Jun 8, 2019 • edited by marvinhagemeister Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jun 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewiggins left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewiggins Jun 8, 2019 • edited by marvinhagemeister Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marvinhagemeister left a comment

Choose a reason for hiding this comment

Uh oh!

robertknight commented Jun 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

andrewiggins commented Jun 8, 2019 •

edited by marvinhagemeister

Loading

coveralls commented Jun 8, 2019 •

edited

Loading

andrewiggins Jun 8, 2019 •

edited by marvinhagemeister

Loading