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

📝 Enable codeblock TS transpilation #4149

Merged

Conversation

Shrugsy
Copy link
Contributor

@Shrugsy Shrugsy commented Aug 3, 2021

  • Set up ability to transpile codeblocks to show a TS & JS tab
  • Convert 'WritingTests' page to use transpiled codeblocks
  • Update snippets on 'WritingTests' page

name: 📖 New/Updated Documentation Content
about: Updating content in an existing docs page

PR Type

Does this PR add a new page, or update an existing page?

Updates an existing page (Writing Tests).
Also adds the ability to transpile doc codeblocks from TS to show both TS & JS tabs.

Checklist

What docs page is being added or updated?

  • Section: Usage
  • Page: Writing Tests

For Updating Existing Content

What updates should be made to the page?

  • Show both TS & JS tabs for codeblocks

Do these updates change any of the assumptions or target audience? If so, how do they change?

Yes - now targeted to accommodate both TS & JS users

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 3, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 886999e:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration

@Shrugsy Shrugsy marked this pull request as draft August 3, 2021 15:35
@netlify
Copy link

netlify bot commented Aug 3, 2021

Deploy Preview for redux-docs ready!

Name Link
🔨 Latest commit 886999e
🔍 Latest deploy log https://app.netlify.com/sites/redux-docs/deploys/62bda9b160075f0007123e9c
😎 Deploy Preview https://deploy-preview-4149--redux-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@timdorr
Copy link
Member

timdorr commented Aug 3, 2021

We shouldn't have the docs path contain any JS files. Instead, can that tooling live in the website path, which is already a package, and just reach out to docs to grab the code?

@Shrugsy
Copy link
Contributor Author

Shrugsy commented Aug 3, 2021

@timdorr

We shouldn't have the docs path contain any JS files. Instead, can that tooling live in the website path, which is already a package, and just reach out to docs to grab the code?

@phryneas has advised:

Could only move it up to the parent folder. It's the normal node module resolution algorithm. Will look up the directory tree into every node module folder, but not left and right

So theoretically it might be possible for all of the packages to stick under website/package.json if the entire docs directory was moved into website

@markerikson
Copy link
Contributor

I definitely don't want to move the docs folder if at all possible.

@Shrugsy Shrugsy force-pushed the docs/enable-ts-codeblock-transpilation branch from 4e1ee88 to f821145 Compare August 4, 2021 07:35
@Shrugsy Shrugsy marked this pull request as ready for review August 4, 2021 07:41
@Shrugsy
Copy link
Contributor Author

Shrugsy commented Aug 4, 2021

@timdorr @markerikson:
@phryneas released an update for remark-typescript-tools, allowing the transformation of the virtual files paths, which in turn allows the docs config to mix in with website (by allowing docs content to resolve node_modules in a sibling directory).

The resultant changes to the docs directory is that code snippets will be written in a manner that enables the TS transpilation & TS/JS tab generation, while all config & installed packages can live under website instead.

I've converted the 'writing tests' page to transpile the codeblocks, which can be previewed here: https://deploy-preview-4149--redux-docs.netlify.app/usage/writing-tests

@timdorr I recommend specifically having a look at the snippet changes involved in WritingTests.mdx to get an idea of how that will affect someone looking at the 'raw markdown'.

Note that the benefit here is more than just creating a TS/JS tab - It also ensures that the snippet is actually valid code. I fixed up a couple of mistakes/missing imports while converting this page thanks to that.

@markerikson
Copy link
Contributor

markerikson commented Aug 4, 2021

Looks pretty good to me!

Do we have full Netlify caching turned on for the core docs site atm, the way we do for RTK?

@Shrugsy
Copy link
Contributor Author

Shrugsy commented Aug 4, 2021

Do we have full Netlify caching turned on for the core docs site atm, the way we do for RTK?

I have no idea personally. I can say that the netlify build did seem fast, but that could be because only this one page has any transpilation going on so far

@timdorr
Copy link
Member

timdorr commented Aug 5, 2021

@timdorr I recommend specifically having a look at the snippet changes involved in WritingTests.mdx to get an idea of how that will affect someone looking at the 'raw markdown'.

Well, the good news is GitHub knows exactly what mdx is and displays it the same as plain Markdown: https://github.com/Shrugsy/redux/blob/docs/enable-ts-codeblock-transpilation/docs/usage/WritingTests.mdx

@Shrugsy
Copy link
Contributor Author

Shrugsy commented Aug 5, 2021

Well, the good news is GitHub knows exactly what mdx is and displays it the same as plain Markdown: https://github.com/Shrugsy/redux/blob/docs/enable-ts-codeblock-transpilation/docs/usage/WritingTests.mdx

Yeah good point. Basically the thing I want to point out as a 'potential concern' is that the raw markdown will include portions in the snippet that are intended for use as virtual files, but not intended to be included on the final snippet (anything marked as noEmit isn't meant to be displayed)

For efficiency's sake, these may often be be partial pieces of code that aren't intended to make much sense for someone reading the docs, but are created to have the correct type signature, and may only include the necessary content that the relevant snippet file wants to import

e.g. below
image

@markerikson
Copy link
Contributor

Oh hey, I completely forgot this was here :)

@Shrugsy: any chance you could rebase this against the changes on master? Unfortunately this collides with the changes I just made to WritingTests.md and upgrading to DS 2.21, so you may end up needing to rework it.

@Shrugsy Shrugsy force-pushed the docs/enable-ts-codeblock-transpilation branch from 63d6a67 to 886999e Compare June 30, 2022 13:48
@Shrugsy
Copy link
Contributor Author

Shrugsy commented Jun 30, 2022

Oh hey, I completely forgot this was here :)

@Shrugsy: any chance you could rebase this against the changes on master? Unfortunately this collides with the changes I just made to WritingTests.md and upgrading to DS 2.21, so you may end up needing to rework it.

@markerikson FYI - I've rebased this branch and adjusted for the collisions.

FYI - I've made some (admittedly opinionated) adjustments to WritingTests while updating the codeblocks for transpilation.

Rather than exporting renderWithProviders as render, and re-exporting everything from RTL, I've kept it as renderWithProviders, and not re-exported anything from RTL. I think it's confusing for users to set it up the other way, adding indirection as to where their test functions are coming from, and what they do. With how well automatic imports are already handled, I don't see a good reason to re-export from RTL. Plus, because of automatic imports, overriding render with a separate function by the same name only provides additional opportunities to accidentally import the wrong one, whereas keeping it as renderWithProviders maintains a clear distinction.

@Shrugsy
Copy link
Contributor Author

Shrugsy commented Jun 30, 2022

For clarity - what this MR really does is allow for the documentation to:

e.g.

Before After
image image

@markerikson
Copy link
Contributor

SWEEEEET let's do this!

@markerikson markerikson merged commit 14f808d into reduxjs:master Jun 30, 2022
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