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

feat: mangle error codes to error indexes #3920

Merged
merged 12 commits into from
Apr 2, 2021

Conversation

andrewmcgivery
Copy link
Contributor

@andrewmcgivery andrewmcgivery commented Oct 25, 2020

PR Type

Does this PR add a new feature, or fix a bug?

Feature (ish)

Why should this PR be included?

This small addition to the build script results in a savings of around 30% when minified/bundled.

Before/After of Redux.min.js:
image
image

Checklist

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Is there an existing issue for this PR?
  • Have the files been linted and formatted?
  • Have the docs been updated to match the changes in the PR? (Need feedback)
  • Have the tests been updated to match the changes in the PR? (Haven't added tests... looking for feedback if this is needed)
  • Have you run the tests locally to confirm they pass?

New Features

What new capabilities does this PR add?

Added mangleErrors babel script which converts error messages from strings to error code indexes per #3655.

For example:

Before:

throw new Error("This is my error message.");
throw new Error("This is a second error message.");

After (with minify):

throw new Error(0);
throw new Error(1);

After (without minify):

throw new Error(node.process.NODE_ENV === 'production' ? 0 : "This is my error message.");
throw new Error(node.process.NODE_ENV === 'production' ? 1 : "This is a second error message.");

What docs changes are needed to explain this?

May need a new doc around debugging in production but hoping the documentation king @markerikson has some ideas here :)

Outstanding Questions

  • Do we need a change in documentation to explain this behavior?
  • Do the error codes need to be documented somewhere or can we just reference the errors.json file?
  • Are we ok with errors.json being in the root directory or should I move this to the /scripts folder?
  • Any concerns with the output of the errors.json file?
  • It appears I may have been the first person into the rollup configuration file with prettier enabled which resulted in A LOT of formatting changes... is this a problem?
  • Is this MR even valuable? I decided to take a crack at it out of interest and wanting to contribute back to redux but I won't be offended if the answer is "nah".

Added mangleErrors babel script which converts error messages from strings to error code indexes.
@netlify
Copy link

netlify bot commented Oct 25, 2020

Deploy preview for redux-docs ready!

Built with commit 1cd8f07

https://deploy-preview-3920--redux-docs.netlify.app

@markerikson
Copy link
Contributor

Hey, this is pretty neat! And yes, saving 2.1K is totally worth it.

First few questions off the top of my head:

  • master is the develop branch for what will, at some point, be Redux 5.0, but we don't have any actual timeline on when that will be released. Is this something we can / should try porting into the v4.x line? If so, is that just a minor version bump?
  • How does the React docs site handle the error decoder? Is that something that's Gatsby specific? Can it be set up to work with Docusaurus?
  • I think TSDX may do something like this. Have we looked at how they handle it?
    • Side thought: is it worth looking at switching to TSDX for building Redux v5 itself?
  • How does this work with varying versions over time? What happens if we add a new error message somewhere? Do we need to keep around each version's error messages list permanently? If so, where / how do we do that?
  • Can we examine doing this for React-Redux and Redux Toolkit as well?

Also paging @phryneas and @msutkowski here

@andrewmcgivery
Copy link
Contributor Author

andrewmcgivery commented Oct 26, 2020

@markerikson Awesome! Putting my 2 cents to your points but I assume the folks you tagged will have some thoughts too!

  • It should be easily portable into 4.x... I can open a separate MR for that after we work out the kinks in this MR. I think it would be a minor bump since there technically isn't any breaking changes to the API itself, assuming we're going off semver here.
  • I've been searching through React to see what they do and it seems like they do some magic to link to a page in their docs (https://reactjs.org/docs/error-decoder.html/, example with an error code passed: https://reactjs.org/docs/error-decoder.html/?invariant=1) (see: https://github.com/facebook/react/blob/75955bf1d7ff6c2c1f4052f4a84dd2ce6944c62e/packages/shared/formatProdErrorMessage.js). Based on their documentation "This file is generated by the Gulp plugin and is used by both the Babel plugin and the error decoder page in our documentation." I'm not completely sure how we could do that here with docusaurus... That would be a new one for me. 😄 (EDIT: Just looked at the code for the redux site, it's VERY custom Gatsby code)
    -Can't answer fully for TSDX but based on their documentation, there would be some refactoring required since "this extraction only works if your error checking/warning is done by a function called invariant", but yes, it does do the same thing as React and the same idea as this. So if you do switch to TSDX, you'll get it out of the box with some refactoring and this custom plugin won't be needed.
    -So the file itself is appended to whenever a NEW error message comes up, old indexes remain in place. My assumption is that the version tags would handle the difference between each version of this file but I may be oversimplifying your question.
    -Absolutely can look at React-Redux and Redux toolkit! Provided they are using a similar build setup with rollup/babel, should be more or less a plug and play. 😄

@andrewmcgivery
Copy link
Contributor Author

So, at the risk of comment spam, I did a quick experiment with docusaurus and it is totally possible to do something with the error codes.... here's an example (excuse the lack of styling) if we just outputted all of them by enumerating over the generated json file:

image

I just created a small React component in website/src/pages/errors.js:

import React from 'react'
import Layout from '@theme/Layout'
import useDocusaurusContext from '@docusaurus/useDocusaurusContext'
import styles from './styles.module.css'
import errorCodes from '../../../errors.json'

function Errors() {
  const context = useDocusaurusContext()
  const { siteConfig = {} } = context

  return (
    <Layout
      title={`${siteConfig.title} - A predictable state container for JavaScript apps.`}
      description="A predictable state container for JavaScript apps."
    >
      <main className={styles.mainFull}>
        <h1>Production Error Codes</h1>
        <p>
          When Redux is built and running in production, error text is replaced
          by indexed error codes to save on bundle size. These error codes can
          be found below.
        </p>
        <table>
          <thead>
            <tr>
              <th>Error Code</th>
              <td>Message</td>
            </tr>
          </thead>
          <tbody>
            {Object.keys(errorCodes).map(code => {
              const message = errorCodes[code]

              return (
                <tr>
                  <td>{code}</td>
                  <td>{message}</td>
                </tr>
              )
            })}
          </tbody>
        </table>
      </main>
    </Layout>
  )
}

export default Errors

@markerikson
Copy link
Contributor

markerikson commented Oct 26, 2020

Sweeeeet! Yeah, that looks like a good starting point. Seems like we ought to be able to build on that.

Someone on Twitter was digging into how React implements things on their side:

https://twitter.com/giovannibenussi/status/1320547654028431360

@andrewmcgivery
Copy link
Contributor Author

andrewmcgivery commented Oct 26, 2020

Sweeeeet! Yeah, that looks like a good starting point. Seems like we ought to be able to build on that.

Someone on Twitter was digging into how React implements things on their side:

https://twitter.com/giovannibenussi/status/1320547654028431360

Yea, I was digging into that code earlier which is where I was saying "Just looked at the code for the redux site, it's VERY custom Gatsby code" in my previous comment. My stuff above with docusaurus is pretty similar to what they are doing.

I think we could do something similar of linking to a page on the docs site, just would be slightly less savings in code as we'd have to add a function that includes the message with the link. So, depends on what we think would be the "best" option for consumers.

@andrewmcgivery
Copy link
Contributor Author

Ok, last comment of the night!

For the repos already using TSDX (Redux Toolkit for example), it's probably best to work with the build tool. This will require some refactoring to replace all throws with an invariant implementation. Seems like tiny-invariant (https://www.npmjs.com/package/tiny-invariant) would be a good choice to keep the bundle size small, or possibly keep a copy of an invariant function in each repo and strip it down even more?

In either case, errors would be refactored:

// Before
throw new Error("This is my error message.");

// After
invariant(false, "This is my error message.");

If we want to explore this in Redux Toolkit, I'm happy to add to my todo list and give it a shot sometime this week. :)

@phryneas
Copy link
Member

phryneas commented Oct 26, 2020

Looks like a very welcome contribution to me.
As for the docusaurus mdx: Keep in mind that that mdx will be evaluated on runtime, it is not part of the SSR. I'm not sure how the json loading will happen - is it inlined or loaded on page load? And I'm not 100% sure how search engines will index that. Should not be a problem, but taking a second look might be a good idea.

@giovannibenussi
Copy link

Hi! I'm the guy from Twitter 😜
Basically for those not familiar with Gatsby, you can define at build time custom GraphQL queries. In this case, they retrieve data from https://raw.githubusercontent.com/facebook/react/master/scripts/error-codes/codes.json and shape it in the following way:

query ErrorPageMarkdown($slug: String!) {
    // other fields
    errorCodesJson {
      internal {
        contentDigest
      }
    }
  }

Basically you don't have to worry about data retrieval in the ErrorDecoder component

<ErrorDecoder
  errorCodesString={data.errorCodesJson.internal.contentDigest}
  location={location}
/>

However, this works well because they retrieve this data once per build and not in real time.

I'm not familiar with redux's docs nor your CI servers, but if you like react docs' solution, just out of my head I can think about copy the contents from errors.json directly from github to your docs' source code in a step previous to build the docs when deploying.

@msutkowski
Copy link
Member

Looks good to me as well! Regarding possibly switching to TSDX, redux exports an ESM browser build which is currently a small challenge to support with TSDX as shown in this PR: reduxjs/redux-toolkit#658. This was also recently referenced in this TSDX issue - outside of that, when I opened that PR all of the custom rollup configs redux uses basically match the TSDX defaults, so it should be pretty straightforward.

@timdorr
Copy link
Member

timdorr commented Oct 26, 2020

We started the TS conversion before TSDX really took off, so it wasn't in the cards. I'd love to switch to it, but obviously that's outside the scope of this PR.

I would like to incorporate this compressed error approach. However, it is technically a breaking change (for production builds, no less), so I don't think it's semver to do this in the 4.x branch. Let's keep this on master and a bonus for those upgrading.

@andrewmcgivery
Copy link
Contributor Author

andrewmcgivery commented Oct 26, 2020

So then for next steps...

  • Is the documentation page for this within the scope of this MR? We have a technical solution but obviously it needs some work. I could also add the skeleton as part of this MR and it could be refined in future MRs.
  • Should we use the React approach to call a function that includes a link to the error page within the production error message instead of just the number?
  • TSDX is NOT in scope for this MR and should be deferred to a future MR.
  • In a separate MR, update error handling in Redux Toolkit (and look into React-Redux as well) to use invariant instead of throw. (assuming this will be considered a breaking change in those libraries as well so I'll need some guidance on what branching strategy to use)

@markerikson Thoughts? 😄

@andrewmcgivery
Copy link
Contributor Author

andrewmcgivery commented Oct 27, 2020

Based on jaredpalmer/tsdx#759 (comment) and jaredpalmer/tsdx#912 (comment) there are some issues in TSDX (more specifically upstream in rollup) that will cause some issues in all redux repos. Seems like the error extraction isn't a well supported feature.

@markerikson
Copy link
Contributor

@andrewmcgivery nice research.

I'd say let's see if we can put together some kind of complete solution in this PR, including generating an appropriate docs page based on a JSON file extracted earlier in the docs build step.

…d build to link to redux error page, styled error page and changed to only show error text for error passed in url.
@andrewmcgivery
Copy link
Contributor Author

andrewmcgivery commented Oct 28, 2020

@markerikson ok so some changes!

Using a shared function that is injected in dynamically just like the React implementation, error messages now output a message with a link to the site with an error code in the URL:

Minified Redux error #${code}; visit https://redux.js.org/Errors?code=${code} for the full message or use the non-minified dev environment for full errors.

Took some heavy inspiration from React's page and tried to make the styles match the docs page as much as possible, looks like this when you follow that URL:

image

Worth mentioning, this is slightly less savings due to the function but still savings:

image

Errors file is automatically updated during build process and the website pulls the error codes from it directly.

Anything missing? :)

EDIT: Seems to be some build issues with the website. Working through that.

Edit 2: Fixed 😄

Edit 3: Apparently the latest VSCode update broke prettier so I had to run the formatter cause it was breaking the build. Fingers crossed it'll pass this time!

@andrewmcgivery
Copy link
Contributor Author

@markerikson Sorry, I know you're busy with the doc rewrites and I don't want to be that guy that bumps a PR... I just don't know the proper protocol to ping you. 😨

@markerikson
Copy link
Contributor

Yeah, thanks for the ping, actually - I missed the last update here.

A couple more questions:

  • Just for kicks, can we throw in the rest of the error codes on this page in case anyone wants to reference them?
  • Does this work right with the UMD prod build?

Personally, I still think we ought to be able to do this for 4.x. @timdorr , I wouldn't see this as a "breaking" change per se, although I can understand why it could be viewed that way. Also, given that we have no concrete plans to release 5.0 any time soon, I'd see this as a net win - shrinking some byte size off the core lib. Say, 4.1.0?

@andrewmcgivery
Copy link
Contributor Author

@markerikson Added the full list of codes to the page. Screenshots of light and dark themes:

image
image

Regarding the UMD prod build, is there some way you'd prefer to verify this? I took a quick look through the minified source code and I do see the minified formatProdErrorMessage function and it being used throughout the code with the error codes instead of the text.

@markerikson
Copy link
Contributor

I could pull and build this myself, but don't feel like doing so atm :) I'll take your word for it for now.

FWIW, I threw the question out on Twitter and over in Reactiflux. Some responses:

I would do that in a major bump I think - it's possible people are relying on the error message format. But it's not a terrible offender if it were in a minor version bump.

same person a minute later:

I think a minor version bump is somewhat reasonable still

Someone else, on Twitter:

I see the gray area but would personally put it as a patch version - error messages shouldn't be considered public, that is what error numbers or codes are for. Having said that, sorta depends on how it was documented and if there were error codes to consume.

Related: https://semver.org/#what-if-i-inadvertently-alter-the-public-api-in-a-way-that-is-not-compliant-with-the-version-number-change-ie-the-code-incorrectly-introduces-a-major-breaking-change-in-a-patch-release

@timdorr
Copy link
Member

timdorr commented Nov 3, 2020

You can use the Netlify preview to see it in action: https://deploy-preview-3920--redux-docs.netlify.app/errors?code=5

@markerikson
Copy link
Contributor

That is slick.

Well, I'm sold. Tim, thoughts?

@timdorr
Copy link
Member

timdorr commented Nov 3, 2020

I think it looks pretty good. Any outstanding TODOs that I may have missed?

@andrewmcgivery
Copy link
Contributor Author

MR is all good to go from my perspective!

The only note I will add is I would be a little hesitant to switch all of the redux repos to TSDX unless there is a good reason to based on the issues around error extraction not working and it being extremely difficult to debug why. I'm sure TSDX is great if you're starting from scratch and don't want to deal with rollup configuration but I would argue if you've already got a config that is working, I don't see the benefit to changing that. Just my 2 cents. :)

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.

7 participants