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

Use consistent module layout and naming #97

Merged
merged 5 commits into from
Feb 12, 2024

Conversation

tilgovi
Copy link
Contributor

@tilgovi tilgovi commented Feb 7, 2024

Move every module in the contexts directory to the components directory. Name each one with a consistent suffix "Context" suffix. From each one, export the context and the context value interface.

For every context module, ensure there is a corresponding module with a component that wraps the provider. These modules do not have any suffix.

Move the useLayoutGroupEffect hook into the hooks directory. Fix all of the remaining references to the old "deferred layout effects" naming.

Export only a single EditorProps type that is the type of the props for the ProseMirror and Editor components. Hooks do not export their types.

Make all of the exports in the barrel module consistent.

Rename PortalRegistry to NodeViews.

@tilgovi tilgovi requested review from smoores-dev and a team as code owners February 7, 2024 19:06
@tilgovi
Copy link
Contributor Author

tilgovi commented Feb 7, 2024

I based this loosely on what react-redux does, where Context and Provider are both in a components directory. It makes sense. Even though we're exporting a context, a context object is an object with Provider and Consumer attributes that are components. So, these are also components. Keeping it all together in the components directory seems simpler.

To get a feel for this change, mostly just look at the changes to src/index.js.

@tilgovi
Copy link
Contributor Author

tilgovi commented Feb 7, 2024

I don't think changing the naming of the portal registry should affect anyone. We don't have that documented as public interface. Mostly, folks should just be calling renderNodeViews. But now useNodeViews is consistent with NodeViews and NodeViewsContext and it's clearer that they all work together.

@tilgovi tilgovi force-pushed the module-layout-and-naming-consistency branch from 476c6bc to d6c85df Compare February 7, 2024 19:10
@tilgovi
Copy link
Contributor Author

tilgovi commented Feb 7, 2024

I actually have a little more to clean up here. Hmm. I am just getting hung up on the React EditorProps vs the ProseMirror EditorProps.

@tilgovi tilgovi force-pushed the module-layout-and-naming-consistency branch from d6c85df to b62437a Compare February 7, 2024 21:31
@tilgovi
Copy link
Contributor Author

tilgovi commented Feb 7, 2024

Okay, this should be ready to go now.

Move every module in the contexts directory to the components directory.
Name each one with a consistent suffix "Context" suffix. From each one,
export the context and the context value interface.

For every context module, ensure there is a corresponding module with a
component that wraps the provider. These modules do not have any suffix.

Move the useLayoutGroupEffect hook into the hooks directory. Fix all of
the remaining references to the old "deferred layout effects" naming.

Export only a single EditorProps type that is the type of the props for
the ProseMirror and Editor components. Hooks do not export their types.

Make all of the exports in the barrel module consistent.

Rename PortalRegistry to NodeViews.
@tilgovi tilgovi force-pushed the module-layout-and-naming-consistency branch from b62437a to b519d9e Compare February 12, 2024 17:41
Copy link

@eelzon eelzon left a comment

Choose a reason for hiding this comment

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

I'm a fan of the export naming changes especially -- makes it a lot easier to read the index.js file when getting oriented in what features are available to import!

Copy link
Collaborator

@smoores-dev smoores-dev left a comment

Choose a reason for hiding this comment

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

I don't think this should be a blocking sentiment, just a slight modification to consider:

It feels a little weird to me that the contexts are in their own files in the components directory. I think I could get behind putting all of the Contexts, split out as they are now, in a contexts directory, and even maybe putting all of the context definitions inside their corresponding component definition files. But contexts are more than just components, and are also used by hooks, which is why it feels a little weird to have them live alongside components.

Anyway, the other comment is more important I think, this one is just something I wanted to throw out there.

src/index.ts Outdated Show resolved Hide resolved
@tilgovi
Copy link
Contributor Author

tilgovi commented Feb 12, 2024

It feels a little weird to me that the contexts are in their own files in the components directory.

I was a bit on the fence, myself, but I've grown to like it. They aren't components, but their .Provider and .Context are. We do call useContext, of course, but even so, contexts are kinda "in" the component tree.

Until APIs stabilize and users demonstrate a need for more control,
remove the layout group and low-level editor exports.
@@ -6,7 +6,7 @@ import { Plugin, PluginKey } from "prosemirror-state";
*/
export const ROOT_NODE_KEY = Symbol("@nytimes/react-prosemirror/root-node-key");

export type NodeKey = string | typeof ROOT_NODE_KEY;
export type NodeKey = string | symbol;
Copy link
Contributor Author

@tilgovi tilgovi Feb 12, 2024

Choose a reason for hiding this comment

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

This was necessary because React doesn't want to let us make an interface type that's indexable by a unique symbol. With this change, our interface can be this:

export interface NodeViewsContextValue {
  [key: NodeKey]: NodeViewRegistration[];
}

@tilgovi
Copy link
Contributor Author

tilgovi commented Feb 12, 2024

If you're unconvinced by the Context argument, I really don't mind moving them back to a contexts.

@smoores-dev
Copy link
Collaborator

If you're unconvinced by the Context argument, I really don't mind moving them back to a contexts.

Hahaha ok, I appreciate that; I am indeed unconvinced.

@tilgovi tilgovi force-pushed the module-layout-and-naming-consistency branch 2 times, most recently from f7c30f1 to 20f93b0 Compare February 12, 2024 19:11
@tilgovi
Copy link
Contributor Author

tilgovi commented Feb 12, 2024

Okay. Contexts are back where you want them!

@smoores-dev
Copy link
Collaborator

I think the readme formatting needs to be adjusted

@tilgovi
Copy link
Contributor Author

tilgovi commented Feb 12, 2024

I think the readme formatting needs to be adjusted

Where are you seeing an issue? I just glanced at the rendered markdown and don't see it.

@tilgovi
Copy link
Contributor Author

tilgovi commented Feb 12, 2024

Oh. Prettier!

@tilgovi tilgovi force-pushed the module-layout-and-naming-consistency branch from 20f93b0 to 7d2beff Compare February 12, 2024 19:16
@tilgovi
Copy link
Contributor Author

tilgovi commented Feb 12, 2024

Okay. We're all green.

@tilgovi
Copy link
Contributor Author

tilgovi commented Feb 12, 2024

@smoores-dev let me know if you find anything else!

@tilgovi tilgovi merged commit 2f40821 into main Feb 12, 2024
2 checks passed
@tilgovi tilgovi deleted the module-layout-and-naming-consistency branch February 12, 2024 19:53
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

3 participants