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

Don't crash when building docs #2379

Conversation

koddsson
Copy link
Contributor

@koddsson koddsson commented Sep 27, 2022

I moved the @github/markdown-toolbar-element import into the render function like suggested in https://www.gatsbyjs.com/docs/debugging-html-builds/

Closes github/markdown-toolbar-element#68

Screenshots

Please provide before/after screenshots for any visual changes

Merge checklist

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

cc/ @keithamus

@koddsson koddsson requested review from a team and pksjce September 27, 2022 13:40
@changeset-bot
Copy link

changeset-bot bot commented Sep 27, 2022

⚠️ No Changeset found

Latest commit: 2db9061

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

Copy link
Member

@keithamus keithamus left a comment

Choose a reason for hiding this comment

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

LGTM

@joshblack
Copy link
Member

Responded on the upstream issue, let me know what you think!

Separately, would this run into the same mix-and-match issue described in: #2339 ? Specifically:

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).

@koddsson
Copy link
Contributor Author

Separately, would this run into the same mix-and-match issue described in: #2339?

I had no issue with the mix of require and import in my testing and I believe that CI should catch any such compiler issues.

@koddsson
Copy link
Contributor Author

koddsson commented Sep 27, 2022

I pushed a commit that utilizes useEffect and dynamic imports so that we can avoid require calls if we want.

Reverted that since nodejs/jest seems to crash with a dynamic import.

@joshblack
Copy link
Member

@koddsson sorry about the dynamic import stuff 😞 Seems like the same issue that @iansan5653 ran into.

For the mix-and-match, this would end up being a bundler behavior, right? If I understand right, require() is undefined in an ESM scope.

Also wanted to check-in with the deploy preview, it seems like the build is failing with this error:

image

@koddsson
Copy link
Contributor Author

Also wanted to check-in with the deploy preview, it seems like the build is failing with this error:

I don't see this anywhere. Do you have a link to this UI?

@joshblack
Copy link
Member

@koddsson let me know if this works! https://github.com/primer/react/actions/runs/3144532666/jobs/5110661581

I think it's showing up for the Deploy (fork) workflow

@koddsson
Copy link
Contributor Author

I annoyingly can't reproduce the deploy error locally. Any ideas? Can you reproduce it?

@koddsson koddsson temporarily deployed to github-pages October 4, 2022 11:18 Inactive
@koddsson
Copy link
Contributor Author

koddsson commented Oct 4, 2022

I annoyingly can't reproduce the deploy error locally. Any ideas? Can you reproduce it?

Seems like this was a issue with me not having push access to this repo. Seems to be running green now 👍🏻

Co-authored-by: Matthew Costabile <[email protected]>
@koddsson koddsson temporarily deployed to github-pages October 5, 2022 08:17 Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2022

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.

@github-actions github-actions bot added the Stale label Dec 4, 2022
@github-actions github-actions bot closed this Dec 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants