Skip to content

Conversation

@jennspencer
Copy link
Contributor

@jennspencer jennspencer commented Jul 2, 2024

PR App Fix RM-10063

🧰 Changes

Unhandled nodes are throwing errors during migration. Offending node types:

  • div
  • embed
  • figure
  • i
  • rdme-pin
  • yaml

🧬 QA & Testing

You can look up the Cannot handle unknown node <node> errors from Aaron's migration spreadsheet to find pages to test locally, if you want to use make mdx to swap out the dist files in your readme repo.

You'll have to check out his other PR with the migration scripts first, which has directions for testing pages.

.use(hast ? rehypeRemark : undefined)
.use(remarkMdx)
.use(remarkGfm)
.use(divTransformer)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm not 100% on this placement, whether or not it should be before the hastifying

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 fine, I don't think there's any hastifing in this function. It's taking either an mdast or a hast and outputting mdx.

@jennspencer jennspencer marked this pull request as ready for review July 5, 2024 23:27
@jennspencer
Copy link
Contributor Author

jennspencer commented Jul 5, 2024

this will cause some of the pages with div node errors to instead error with Expected component 'TutorialTile' to be defined: you likely forgot to import, pass, or provide it.

i'm assuming that's because custom components aren't being sent in the migration script--easy enough to fix

@kellyjosephprice
Copy link
Collaborator

this will cause some of the pages with div node errors to instead error with Expected component 'TutorialTile' to be defined: you likely forgot to import, pass, or provide it.

i'm assuming that's because custom components aren't being sent in the migration script--easy enough to fix

Yea, it's defined in the main app. We should probably add a stub to the migration so we don't got those errors.

Copy link
Collaborator

@kellyjosephprice kellyjosephprice left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me.

Thanks for cleaning up all the cruft!

type: NodeTypes.tutorialTile,
} as TutorialTile;
parent.children.splice(index, 1, tile);
// idk what this is and/or just make it a paragraph
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤷🏼

@jennspencer jennspencer merged commit 5d21b54 into beta Jul 8, 2024
@jennspencer jennspencer deleted the jenn/rm-10063-convert-unknown-nodes branch July 8, 2024 17:55
rafegoldberg pushed a commit that referenced this pull request Jul 8, 2024
## Version 6.75.0-beta.70

### 🛠 Fixes & Updates

* compatibility for unknown nodes ([#929](#929)) ([5d21b54](5d21b54))

<!--SKIP CI-->
@rafegoldberg
Copy link
Contributor

This PR was released!

🚀 Changes included in v6.75.0-beta.70

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants