Remove & inline markdown-toolbar-element dependency

[iansan5653]

In the MarkdownEditor, we are using markdown-toolbar-element to power basic formatting tools (bold, italics, etc). However, this is a custom element and not a React component, so this strategy comes with some complications.

The way we are using the element is by rendering it with display: none and then calling .click() on the underlying elements. This is really hacky but it's necessary because the markdown-toolbar-element package doesn't expose the underlying logic at all.

In addition to the hackiness, the dependency is problematic because it performs side effects (registering custom elements) on import, so we have to conditionally require the dependency in a useEffect callback to avoid calling browser APIs during server-side rendering. This mix of require and import confuses transpilers and occasionally breaks things in unexpected and hard-to-debug ways (for example, see this Slack thread).

To get around this issue, I tried using require() inside a useEffect instead (pull request). This fails due to a known bug in the JavaScript engine that causes a segmentation fault.

---

So to resolve both the hackiness of this approach and the issues with SSR/unit tests, I've decided to remove the dependency altogether and inline the logic instead. The downside to this is that we are duplicating the logic in that dependency when the whole point of the dependency was to help remove duplication, but there are many upsides.

While inlining this logic, I've tried to make as few changes as possible in order to reduce risk. This means that I left some refactoring opportunities on the table for future work - I've only made the following changes:

1. Removed all code relating to custom elements, turning the entire thing into a hook instead
2. Removed the insertText function in favor of using our much more robust syntheticallyChangeInput strategy. This required changing the logic inside formatting functions a bit to avoid directly passing around / mutating the HTMLTextareaElement instance.
3. Replaced the logic inside expandSelectionToLine with the getSelectedLineRange util, since this was an easy and simple win for removing duplication
4. Renamed some functions, arguments, and types

---

An even better solution would be to extract all the logic that's shared between the PVC and PRC Markdown editors into a new common behavior dependency. This could include formatting, list editing, file handling, etc... But this is not something that I have the capacity for at the moment, and I don't want to delay this fix as it's actively blocking work on downstream teams.

Screenshots

There are no visual changes.

Merge checklist

• [ ] Added/updated tests
• [ ] Added/updated documentation
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

[changeset-bot]

🦋 Changeset detected

Latest commit: 151c010

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	76.3 KB (0%)
dist/browser.umd.js	76.94 KB (0%)

[Lukeghenco]

Work looks great, but there appears to be some issues with act() in the tests that are failing with the changes.

[iansan5653]

Work looks great, but there appears to be some issues with act() in the tests that are failing with the changes.

I think my definition for taskListStyle was wrong and that's causing the failures. The act warnings aren't new (if I remember right). We'll see if the latest commit passes

[Lukeghenco]

Changes appear to have fixed the broken CI build

[keithamus]

I don’t think we want to do this for several reasons. Chiefly being that we should be leaning into web components more within Primer React as we do with Primer View Components, because they offer us useful tools within the browser environment.

Moving away from sharing dependencies will cause inconsistencies across the two design systems. You mention sharing logic in behaviours, I think that’s definitely the plan - but I also think as behaviours get more complex they’ll start to take the shape of web components more and more, so we’ll have to address it there too.

The issues you mention around specifics of markdown toolbar sound imminently fixable. We should address those so we can continue to use this component.

Issues around compiler tool chains sound interesting. Could you detail the specific issues you’re having here? It sounds like something that needs to be addressed in our tool chain.

[iansan5653]

we should be leaning into web components more within Primer React as we do with Primer View Components

I have to disagree with you there. I think the web components pattern works well with PVC, but the same cannot be said for React. While web components technically can be used within React, they come with so many downsides that it's just not worth it.

Specifically looking at this markdown-toolbar-element, here are a few of the problems I've encountered:

• The buttons inside the toolbar don't use the PRC Button component. In fact they can't, because web components can't use React components inside. This means that we can't make the toolbar buttons look consistent with the rest of React buttons. They will never be able to access the React theme or reuse the base components.
• The toolbar manages its own focus zone. But PRC already has tools for managing focus zones (like the useFocusZone hook). It's impossible to override the built-in focus functionality so we cannot make useFocusZone work with this toolbar.
• The toolbar is incompatible with SSR because it auto-registers itself on import. We could fix this by providing a register function that gets called in the client, but it will never be fully compatible with SSR because web components are rendered client-side.
• If we want to programmatically perform formatting actions on the editor in React code, the only way to do so is by simulating a click event on the corresponding toolbar button. This is ugly and difficult to maintain. An example of this is binding keyboard shortcuts to the input.
• None of the internal logic is exposed through the element, so we can't reuse any of it.
• The element doesn't public any TypeScript types, so we had to define them in PRC.

While some of these problems are fixable, most are not because they stem from a fundamental incompatibility between web components and React components. They just don't work well together because they align with two very different styles of web architecture.

I think sharing the logic code is great and we should definitely do that as much as possible. But React components should be allowed to handle their own rendering the way they want, using React patterns and React components internally. The logic code should be purely logic code - it shouldn't come with rendered baggage.

As we as a company start to move in a React-first direction, it just doesn't make sense to be trying to make web components fit here. We should try to make it as easy as possible to share logic between our different component systems, and web components don't do that.

I also think as behaviors get more complex they’ll start to take the shape of web components more and more

I don't think this is necessarily the case. As behaviors get more complex, tying them into rendered components feels like an antipattern. Having a simple behavior tied to a component is one thing, but complex behaviors should be as extensible and customizeable as possible.

---

In my ideal world, the PVC and PRC implementations of the Markdown editor would share no rendered components at all, but would share as much of the underlying logic as they possibly can (ie, formatting tools, file handling, suggestions filtering, etc). This way the React implementation can be rendered purely using React, gaining all of the advantages that React provides without any hacky workarounds to use the shared logic.

[keithamus]

• The buttons […] means that we can't make the toolbar buttons look consistent with the rest of React buttons. They will never be able to access the React theme or reuse the base components.

Yeah there’s work to do here to get React to utilise native CSS which is being worked on. The hope is certainly that these will end up using the same themes and same base components.

• The toolbar manages its own focus zone. But PRC already has tools for managing focus zones (like the useFocusZone hook). It's impossible to override the built-in focus functionality so we cannot make useFocusZone work with this toolbar.

The useFocusZone hook is specific to React but should move to be a shared primitive which both will use. There’s work happening in web standards to have a new element represent a focus group and we should align our work to this.

• The toolbar is incompatible with SSR because it auto-registers itself on import. We could fix this by providing a register function that gets called in the client, but it will never be fully compatible with SSR because web components are rendered client-side.

The Markdown toolbar component doesn’t do any client side rendering IIRC, the tags can be rendered by any server. The client side bundle needs specific client side code just like react does with react-dom. This should be reasonably straightforward to deliver for Primer as we do on GitHub. If there are issues around delivering this we should get them fixed; this is all standard JavaScript so everything should be interoperable and if it fails to be we should remediate that.

• If we want to programmatically perform formatting actions on the editor in React code, the only way to do so is by simulating a click event on the corresponding toolbar button. This is ugly and difficult to maintain. An example of this is binding keyboard shortcuts to the input.

There’s lots of ways to solve this! We should explore them!

Just like other behaviours, shortcut binding should be handled consistently between platforms we have.

• None of the internal logic is exposed through the element, so we can't reuse any of it.

Right. Let’s fix that to make sure it is reusable. We own both repositories so we can get this done in a reasonable time frame.

• The element doesn't public any TypeScript types, so we had to define them in PRC.

The components are written in typescript and have declared types: https://github.com/github/markdown-toolbar-element/blob/e48fbc5b8aec1f0451325b616d45f1324cdbce36/src/index.ts#L2. Perhaps there’s an error in the publish or perhaps typescript is misconfigured? We should look into this because this is definitely imminently solvable.

I think sharing the logic code is great and we should definitely do that as much as possible. But React components should be allowed to handle their own rendering the way they want, using React patterns and React components internally. The logic code should be purely logic code - it shouldn't come with rendered baggage.

It’s great that we’re in agreement here! Our web components including markdown toolbar render very little and are very nicely composable. Web components are just standard JavaScript so should work fine with any framework.

As we as a company start to move in a React-first direction, it just doesn't make sense to be trying to make web components fit here. We should try to make it as easy as possible to share logic between our different component systems, and web components don't do that.

We aren’t moving in a React-first direction and even if we were web components are still a great fit for sharing logic between different component systems which is exactly why we’re looking more into doing exactly that.

I don't think this is necessarily the case. As behaviors get more complex, tying them into rendered components feels like an antipattern. Having a simple behavior tied to a component is one thing, but complex behaviors should be as extensible and customizeable as possible.

We’re in agreement here! Components should be extensible and customisable. This is about how we deliver patterns and technology choices matter very little here.

Again I think any specific issues with the markdown toolbar element should be resolved, and we should work to ensure we can share as much as possible between our two platforms to ensure consistency. I think it’d benefit us greatly here to talk about just the specific issues with markdown toolbar element so we can resolve those and move forward productively!

[Lukeghenco]

Thanks @keithamus for your thoughts here and I appreciate your perspective. Just for posterity sake, this code approach was discussed more broadly in this Slack Convo with @mattcosta7 and @dgreif.

Disclaimer: The following is my POV and what I would want from the perspective of a React developer.

I agree with @iansan5653 stated points on this and he did an incredible job breaking down why he choose this solution. Web Components and React Components work together for simple tasks from my experience, but requiring them to work together and blocking this Pull Request seems like the wrong approach to me. My team has attempted to use some web components features in the past, but we found that having more control over the components in React were easier to use, customize and test than using the Web Component primitives. We have also removed all (or most) of the web components in our project. Offering extensibility in the React code for features we will be using feels like the right approach and the approach I believe most React developers will prefer. I think having to access imperative DOM APIs to interact with Web Component and needing refs to interact with the DOM nodes directly is not a great experience and not an experience that I would be happy using.

Blocking this PR will also continue requiring some repos to have a dependency on @primer/react and a @primer/react *canary release. The component exists in PRC now as a drafts component, but it breaks our tests (which is what this PR ultimately resolves).

I feel strongly that we should have reasonable expectations on what we require for developers to share their components upstream to PRC. The ask here, to me, seems like a high bar for @iansan5653 to assess and re-work (if we decide this is the best approach) as someone that is not a core contributor to the Primer React or Web Component repositories. Teams that are in need of this component requested this port from @iansan5653's team. Which he has graciously done with this work. I think to continue fostering teams to share their React components for other teams to enjoy, we need to find a balance of making the barrier to entry much easier.

I've personally not seen great experiences or examples of Web Components and React paradigms working well together and I believe that is 100% ok. I will argue that having shared React components that are accessible, easy to build and quickly shareable in this repository or others supersedes requiring them to work with both React and Web Component.

[mattcosta7]

Mostly just adding some commentary to a few points in the comments above since I was tagged here

I don’t think we want to do this for several reasons. Chiefly being that we should be leaning into web components more within Primer React as we do with Primer View Components, because they offer us useful tools within the browser environment.

Conceptually, I agree with this, however the support for web components in react is just not really that good yet. Most of these shortcomings can be worked around, but not very idiomatically. React 19+ should fix this, assuming React releases it on that release. React 17 is still the most commonly used version at github, with the same flaws as React 18 from these tests.

I guess the important question is - do we ship this knowing we unblock some use-cases now or hold off, keeping those trying to use these features blocked until the underlying issues are resolved?

Moving away from sharing dependencies will cause inconsistencies across the two design systems. You mention sharing logic in behaviours, I think that’s definitely the plan - but I also think as behaviours get more complex they’ll start to take the shape of web components more and more, so we’ll have to address it there too.

Agreed there, but also not sure it's worth blocking progress here for the time being, until someone can pick that work up?

The issues you mention around specifics of markdown toolbar sound imminently fixable. We should address those so we can continue to use this component.

I don't think, given the issues we can't yet address (theming, for one) that sounds like a path forward here - without the workarounds mentioned which don't seem worth it to me on the surface

Issues around compiler tool chains sound interesting. Could you detail the specific issues you’re having here? It sounds like something that needs to be addressed in our tool chain.

I think the majority of issues here lie in the fact that primer bundles a commonjs build that attempts to require its dependencies, some of which are esm only builds - leading to runtime errors that can't be rectified just in the toolchains. Primer/react could modify its build system to bundle the dependencies it relies on or stop shipping any commonjs (preferable I think), but I'm not aware of what the deprecations plan would ideally look like there.

The other issues around calling window.customElement.define() in module scope (where window is not defined in non-browser environments) are more easily fixed by enforcing some kind of registration that isn't in a side effect, and/or ensures that typeof window !== undefined before attempting to access it here. Generally, I'd suggest both together

Yeah there’s work to do here to get React to utilise native CSS which is being worked on. The hope is certainly that these will end up using the same themes and same base components.

While this does seem like a goal that's understood as necessary, I'm curious what the timeframe is for this. Weeks to months sounds likelier than days to weeks (I don't believe there's ongoing work towards this yet, but maybe it is).

Web components are just standard JavaScript so should work fine with any framework

except the many cases where they don't work with react well out of the box, or where they require very non-idiomatic escape hatches to work with. Maybe we need to implement a react friendly wrapper, and still hope to not run into the issues that need code that just isn't react friendly.

The element doesn't public any TypeScript types, so we had to define them in PRC.

It looks like it does define types, but it doesn't define JSX.Element types, so needs to have them manually written. The library could define these (and probably should if usage in react is expected)

I think @Lukeghenco sums up a lot of my thoughts here well with the sentiments of:

Offering extensibility in the React code for features we will be using feels like the right approach and the approach I believe most React developers will prefer. I think having to access imperative DOM APIs to interact with Web Component and needing refs to interact with the DOM nodes directly is not a great experience and not an experience that I would be happy using.

We should probably try to provide this level of support for web components used in react, however I think that will require a good deal of effort to do well, while providing an idiomatic way to interact with them (that's also typesafe)

I feel strongly that we should have reasonable expectations on what we require for developers to share their components upstream to PRC.

I think we might be at a good place to consider this a reasonable path, for now, even if we accept that we might need to re-work some of this later. That seems to be what the 'drafts' in prc are for. quick iteration, learning and eventual refining

I've personally not seen great experiences or examples of Web Components and React paradigms working well together and I believe that is 100% ok.

It can happen, but agreed it is not the norm, and not something that just works. This is a failing of react more than anything, but a reality we need to accept some for the time being. We're also limited by our other react technical choices (theming for one) and until those underlying issues are handled, some latitude to work around them while maintaining productivity seems reasonable

[Lukeghenco]

Thanks @mattcosta7 for your thoughtful responses to both me and @keithamus concerns here.

My biggest concern here is this being a blocked PR for reasons I worry it shouldn't be blocked for. I think the solution is a good enough solution for now and I think it is very likely to assume this components internal workings will change a lot in the next weeks/months.

As for this comment:

This is a failing of react more than anything, but a reality we need to accept some for the time being.

I am glad that React is working to improve the interoperability experience with web components though, since they are here to stay as good or bad as that might be depending on your POV of them being standardized as a web standard. I think that React improving this is a win for everyone in web development though.

[keithamus]

Can we please focus on the specific issues we're currently facing, so they can be remediated? Definitely don't want to block anyone but I cannot see what this is fundamentally solving.

[iansan5653]

@keithamus there are two fundamental issues here:

First is the immediate issue, that markdown-toolbar-element calls DOM APIs at the top level. This breaks any rendering outside the browser (ie, server side rendering) because the browser APIs will not be available yet. I see the following solutions - feel free to propose another if you can think of one:

1. ❌ Wait to import the file until after the React component has mounted in the real browser environment, using a require() call inside a useEffect in the component. This is what PRC is doing right now and it is breaking builds because of the mix between require and import. This breaking is understandable considering the two shouldn't be mixed.
2. ❌ Use the same strategy as (1) but use import() instead. This doesn't work because import() (in the form import(x), not in the top-level import x form) causes segmentation faults in the v8 runtime.
3. Move the import to the top-level (as your pull request does) and change the strategy for registering elements in the markdown-toolbar-element source. This requires markdown-toolbar-element to export some sort of a register() function that contains all the DOM API calls. Then we would call that register() function where we are calling require() today (in useEffect).
4. Completely inline the markdown-toolbar-element dependency as this PR does.
5. Move all the underlying logic out of the markdown-toolbar-element into a new "behaviour" library and depend on that instead, removing the markdown-toolbar-element dependency.

Only (3), (4), and (5) are actually viable options here, since (1) and (2) are not working. So we either need to make breaking changes to markdown-toolbar-element, merge this PR, or take the resources to complete (5).

---

There is another less immediate issue at hand here, which is that the markdown-toolbar-element isn't really working for us in the first place. The way it's actually being used in the MarkdownEditor component is by rendering it as display:none and programmatically 'clicking' the buttons so that we can access the formatting functionality:

react/src/drafts/MarkdownEditor/_FormattingTools.tsx

Lines 26 to 74 in 9eaa37e

	export const FormattingTools = forwardRef<FormattingTools, {forInputId: string}>(({forInputId}, forwadedRef) => {
	useEffect(() => {
	// requiring this module will register the custom element; we don't want to do that until the component mounts in the DOM
	if (!hasRegisteredToolbarElement) require('@github/markdown-toolbar-element')
	hasRegisteredToolbarElement = true
	}, [])
	const headerRef = useRef<HTMLElement>(null)
	const boldRef = useRef<HTMLElement>(null)
	const italicRef = useRef<HTMLElement>(null)
	const quoteRef = useRef<HTMLElement>(null)
	const codeRef = useRef<HTMLElement>(null)
	const linkRef = useRef<HTMLElement>(null)
	const unorderedListRef = useRef<HTMLElement>(null)
	const orderedListRef = useRef<HTMLElement>(null)
	const taskListRef = useRef<HTMLElement>(null)
	const mentionRef = useRef<HTMLElement>(null)
	const referenceRef = useRef<HTMLElement>(null)
	useImperativeHandle(forwadedRef, () => ({
	header: () => headerRef.current?.click(),
	bold: () => boldRef.current?.click(),
	italic: () => italicRef.current?.click(),
	quote: () => quoteRef.current?.click(),
	code: () => codeRef.current?.click(),
	link: () => linkRef.current?.click(),
	unorderedList: () => unorderedListRef.current?.click(),
	orderedList: () => orderedListRef.current?.click(),
	taskList: () => taskListRef.current?.click(),
	mention: () => mentionRef.current?.click(),
	reference: () => referenceRef.current?.click()
	}))
	return (
	<markdown-toolbar for={forInputId} style={{display: 'none'}}>
	<md-header ref={headerRef} />
	<md-bold ref={boldRef} />
	<md-italic ref={italicRef} />
	<md-quote ref={quoteRef} />
	<md-code ref={codeRef} />
	<md-link ref={linkRef} />
	<md-unordered-list ref={unorderedListRef} />
	<md-ordered-list ref={orderedListRef} />
	<md-task-list ref={taskListRef} />
	<md-mention ref={mentionRef} />
	<md-ref ref={referenceRef} />
	</markdown-toolbar>
	)
	})

This is defeating the entire purpose of a web component, which is that it renders content to the DOM - it would be much cleaner to have a markdown-formatting behaviour library from which we could import all of these functions.

Why we are doing this is a bit more complicated to explain but it comes down to needing to just use React components in React components. We initially did try (for a long time) to render the toolbar as a toolbar but ran into a myriad of issues that prevented this from working, all of which stem from the fact that the toolbar and its buttons are too different from the corresponding React components. It's far easier to just render a set of IconButton components and manage their focus with useFocusZone than is is to make markdown-toolbar-element look and act like a React component.

---

So that's why I opened this PR. Option (4) not only avoids breaking changes upstream, it also cleans up this hacky code. I think (5) is the obvious best solution, but I don't have the capacity to implement it at the moment and this fix is somewhat urgent.

[keithamus]

Thanks for outlining this @iansan5653. We're actively discussing this and we'll get back shortly.

I think solution 3 should be workable without making breaking changes to either codebase, as the code will run safely client side and server side can be stubbed out if we need to run server side.

The concerns we have with solution 4 is that it may introduce more technical debt with duplicate code which is why we're hesitant to merge.

[iansan5653]

I think solution 3 should be workable without making breaking changes to either codebase, as the code will run safely client side and server side can be stubbed out if we need to run server side.

I'm interested to see what this might look like as a non-breaking change. From my admittedly limited understanding of SSR, I'm not sure if this is as simple as "don't register unless the window object is defined" or if it's more like "wait to register until the window object is defined". First is definitely easy, second might be more challenging.

[keithamus]

Custom Elements don't need to be registered server side so the first one is applicable.

[dgreif]

From an SSR perspective, adding a simple typeof window !== 'undefined' guard should be sufficient. However, that won't solve the fact that the markdown-toolbar-element currently executes with side-effects, and thus cannot be tree-shaken. This means bundles in dotcom that do not need toolbar element will carry it as extra weight that is not needed. We would need to add a register() function to allow tree shaking, which would be a breaking change on the web component package. I think tree shaking is a valid reason for a breaking change here, so maybe that's the right path forward?

[keithamus]

I think the best approach may be as I described in #2353 - which is to have a client side bundle that can pull in these dependencies:

We may want to move all web component definitions to a central file; github/github could de-duplicate these imports as it already imports them via PVC or via direct imports.

[dgreif]

a client side bundle that can pull in these dependencies

This is an interesting idea, but also sounds a lot like the conditional require that is causing issues already. Would the PRC consumer be responsible for the conditional loading?

[keithamus]

Would the PRC consumer be responsible for the conditional loading?

To me that would be the most sensible idea. It’s highly likely that consumers of PRC are already building a client side bundle with client side only code such as polyfills. Also likely they aren’t bundling for server side as it’s not super compelling to do so.

[mattcosta7]

Would the PRC consumer be responsible for the conditional loading?

To me that would be the most sensible idea. It’s highly likely that consumers of PRC are already building a client side bundle with client side only code such as polyfills.

I don't think it's a common approach to making consumers bundle separately for those environments, where possible not to, and might lead to some friction in doing so.

A separate import of like

import "@primer/react/custom-elements" could be added somewhere, and handle it's own type of loading, otherwise consumers will need to either accept a bundlesize hit for web components they don't care about, or need to know which custom-elements to import where.

It does sound like those provide a bit of a rough way to interact with, vs having the import in primer code directly, handling environments gracefully and making this an experience consumers don't need to consider

I'm also wonder whether there's potential namespacing issues (because web-component namespacing isn't a thing) and this isn't just consumed internally. What do consumers who have a md-toolbar or whatever already on the page do? How do we warn them they have something defined already that isn't expected here?

I think in this case it's a bit more complicated because the elements area hidden implementation detail. consumers of primer/react don't even know these exist - will they even understand the errors if they run into them?

Also likely they aren’t bundling for server side as it’s not super compelling to do so.

is that true? I'm not sure how many common ssr frameworks handle this, since it's been a long time since I've had to look at it, there's definitely at least some build that might be used in both client and server. Most react libraries, at least historically, take the 'verify if window exists' approach and specifically move window apis into useEffects to avoid issues around that. Would acting differently here be confusing for consumers who just expect it to work

[iansan5653]

I've opened an issue in github/markdown-toolbar-element#68 where we can discuss strategies for resolving this problem with the element itself.

[joshblack]

Just wanted to follow-up on this PR after some of the discussions on Slack today to see if we're aligned on next steps 👀

It seems like there might be a chance that the underlying require() issue from 35.9.0 is addressed with the new bundling approach we have. If so, that should unblock the existing implementation while we figure out the best approach for this stuff.

In terms of next steps then, what do folks think of:

• Investigate if the v35.11.0 RC addresses the initial require() issue
• If so, no further action needed to unblock package updates
• Either way, it seems like we need to investigate on the PRC side an alternative to import(). It seems like option 3 from Remove & inline markdown-toolbar-element dependency #2339 (comment) was the next best path to try?

[github-actions]

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.