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: error-decoder #6214

Merged
merged 19 commits into from Jan 12, 2024
Merged

Feat: error-decoder #6214

merged 19 commits into from Jan 12, 2024

Conversation

SukkaW
Copy link
Contributor

@SukkaW SukkaW commented Aug 11, 2023

@github-actions
Copy link

github-actions bot commented Aug 11, 2023

Size changes

📦 Next.js Bundle Analysis for react-dev

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 103.88 KB (🟡 +27 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

New Pages Added

The following pages were added to the bundle from the code in this PR:

Page Size (compressed) First Load
/errors 78.19 KB 182.07 KB
/errors/[errorCode] 78.17 KB 182.04 KB

Three Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page Size (compressed) First Load
/404 78.01 KB (🟡 +647 B) 181.89 KB
/500 78.01 KB (🟡 +646 B) 181.88 KB
/[[...markdownPath]] 79.6 KB (🟡 +648 B) 183.48 KB
Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 10% or more, there will be a red status indicator applied, indicating that special attention should be given to this.

@SukkaW SukkaW changed the title Feat: port error-decoder [WIP] Feat: port error-decoder Aug 11, 2023
@SukkaW SukkaW changed the title [WIP] Feat: port error-decoder [WIP] Feat: error-decoder Aug 11, 2023
Comment on lines 59 to 73
const msg = useMemo(() => {
if (!errorMessages?.includes('%s')) {
return errorMessages;
}

if (typeof window !== 'undefined' && isReady) {
return replaceArgs(
errorMessages,
parseQueryString(query),
'[missing argument]'
);
}

return replaceArgs(errorMessages, [], '[parsing argument]');
}, [errorMessages, isReady, query]);
Copy link
Contributor Author

@SukkaW SukkaW Aug 14, 2023

Choose a reason for hiding this comment

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

@rickhanlonii

Note that although the page will be SSG-ed at the build time, the router.query still requires client-side rendering and will only be populated after the initial hydration, thus isReady can not be avoided.

Copy link
Member

Choose a reason for hiding this comment

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

When the query changes, won't that trigger an update? Why is the isReady needed?

Copy link
Contributor Author

@SukkaW SukkaW Aug 18, 2023

Choose a reason for hiding this comment

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

When the query changes, won't that trigger an update? Why is the isReady needed?

Only when isReady is true cab

Per Next.js documentation,: https://nextjs.org/docs/pages/building-your-application/rendering/automatic-static-optimization#how-it-works

To be able to distinguish if the query is fully updated and ready for use, you can leverage the isReady field on next/router.

This also prevents SSR hydration mismatch errors. On the server, Next.js SSR the page without the query, and Next.js will also use the empty query object during the initial hydration.

The isReady: true will only be set after the initial hydration has finished and the query object has been populated (the isReady here acts similarly with React's two-pass rendering approach).

Copy link
Member

Choose a reason for hiding this comment

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

Ok that's a bummer. I also see that there are multiple updates that happen before query and isReady are set, delaying the time it takes to render the final message.

Instead, why don't we read the query params ourselves instead of from the router. That will allow us to set the message in an effect, which fires immediately after hydration, and that will set it at the first moment possible.

Finally, we can add a fade in animation to avoid the flash of unparsed args. Something like this:

function parseQueryString(search: string): Array<string> {
  const rawQueryString = search.substring(1);
  if (!rawQueryString) {
    return [];
  }

  let args = [];

  const queries = rawQueryString.split('&');
  for (let i = 0; i < queries.length; i++) {
    const query = decodeURIComponent(queries[i]);
    if (query.indexOf('args[') === 0) {
      args.push(query.slice(query.indexOf(']=') + 2));
    }
  }

  return args;
}

export default function ErrorDecoder({errorMessages}: ErrorDecoderProps) {
  const [message, setMessage] = useState<string | null>(errorMessages);

  useEffect(() => {
    if (errorMessages == null) {
      return;
    }
    setMessage(
      replaceArgs(
        errorMessages,
        parseQueryString(window.location.search || ''),
        '[missing argument]'
      )
    );
  }, [errorMessages]);

  const displayMessage = useMemo(() => {
    if (!message) {
      return null;
    }
    return urlify(
      replaceArgs(message, parseQueryString(''), '[missing argument]')
    );
  }, [message]);

  if (!displayMessage) {
    return (
      <p>
        When you encounter an error, you{"'"}ll receive a link to this page for
        that specific error and we{"'"}ll show you the full error text.
      </p>
    );
  }

  return (
    <div>
      <p>
        <b>The full text of the error you just encountered is:</b>
      </p>
      <code className="block bg-red-100 text-red-600 py-4 px-6 mt-5 rounded-lg animate-fade-up">
        <b>{displayMessage}</b>
      </code>
    </div>
  );
}
// add to tailwind.config.js
{
  theme: {
    extend: {
      animation: {
        'fade-up': 'fade-up 1s both',
        // ...
      } ,
      keyframes: {
        'fade-up': {
          '0%': {
            opacity: '0',
            transform: 'translateY(2rem)',
          },
          '100%': {
            opacity: '1',
            transform: 'translateY(0)',
          },
        },
        // ...
      }
      // ...
    }
    // ...
  }

Here's what the animation looks like:

Screen.Recording.2023-08-18.at.2.58.40.PM.mov

Copy link
Contributor Author

@SukkaW SukkaW Aug 19, 2023

Choose a reason for hiding this comment

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

@rickhanlonii Done in 4383ac9 (#6214)

Note that, inside useEffect I add an early return if the errorMessages doesn't contain %s. This should avoid extra re-renders for most of the errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rickhanlonii Would you like to review the change in 4383ac9 (#6214)?

In the meantime, I am going to start addressing <ErrorDecoder> for MDX you have mentioned.

@rickhanlonii
Copy link
Member

I pushed facebook/react#27240 to switch the error messages over to this format.

@SukkaW
Copy link
Contributor Author

SukkaW commented Aug 29, 2023

@rickhanlonii

I have implemented the content/errors/[error_code].md feature. Would you like to review it?

@SukkaW SukkaW marked this pull request as ready for review October 9, 2023 06:25
@SukkaW SukkaW changed the title [WIP] Feat: error-decoder Feat: error-decoder Oct 12, 2023
@rickhanlonii
Copy link
Member

Ok, this is great, I pushed some updates:

  • Refactored to a single compileMDX function used by [errorCode] and [[…markdownPath]].
  • Removed the animation because it’s distracting. Instead, if there are no params we just display the error. If there are params, we delay showing the error until they’re inserted, like the old site did.
  • Moved some of the text that needs translated into the markdown files, so the ErrorDecoder only shows the non-translatable error content.
  • Added a generic.md separate from index.md. We may want to do more with index.md like list common errors with links to the page, so this keeps them separate, and I updated the content in index.md.
  • Renamed errorMessages to errorMessage since it’s singular
  • Added errorCode to the ErrorDecoderContext because I plan to use it soon. I have a WIP that uses it but I don’t want to block on it.
  • Moved ErrorDecoderContext out of /MDX, since it’s not needed in MDX files

mdx = fs.readFileSync(rootDir + '/generic.md', 'utf8');
}

const {content, toc, meta} = await compileMDX(mdx, path, {code, errorCodes});
Copy link
Member

Choose a reason for hiding this comment

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

I also added the errorCodes to the cache, since when new errors are pushed upstream we need to break the cache and rebuild the pages. It's not often, but we do change the error message text.

@rickhanlonii rickhanlonii merged commit 8d2664b into reactjs:main Jan 12, 2024
4 checks passed
rickhanlonii added a commit to facebook/react that referenced this pull request Jan 18, 2024
Updates the error decoder to the URL for the new docs site.

- Switches the domain from reactjs.org to react.dev
- Switches to put the error code in the URL for SSG
- All params are still in the query

Example without args:

- Before: `https://reactjs.org/docs/error-decoder.html?invariant=200`
- After: ` https://react.dev/errors/200`

Example with args:
- Before:
`https://reactjs.org/docs/error-decoder.html?invariant=124?args[]=foo&args[]=bar
`
- After: ` https://react.dev/errors/124?args[]=foo&args[]=bar`


Requires: reactjs/react.dev#6214

---------

Co-authored-by: Jan Kassens <jkassens@meta.com>
github-actions bot pushed a commit to facebook/react that referenced this pull request Jan 18, 2024
Updates the error decoder to the URL for the new docs site.

- Switches the domain from reactjs.org to react.dev
- Switches to put the error code in the URL for SSG
- All params are still in the query

Example without args:

- Before: `https://reactjs.org/docs/error-decoder.html?invariant=200`
- After: ` https://react.dev/errors/200`

Example with args:
- Before:
`https://reactjs.org/docs/error-decoder.html?invariant=124?args[]=foo&args[]=bar
`
- After: ` https://react.dev/errors/124?args[]=foo&args[]=bar`

Requires: reactjs/react.dev#6214

---------

Co-authored-by: Jan Kassens <jkassens@meta.com>

DiffTrain build for [b300304](b300304)
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
Updates the error decoder to the URL for the new docs site.

- Switches the domain from reactjs.org to react.dev
- Switches to put the error code in the URL for SSG
- All params are still in the query

Example without args:

- Before: `https://reactjs.org/docs/error-decoder.html?invariant=200`
- After: ` https://react.dev/errors/200`

Example with args:
- Before:
`https://reactjs.org/docs/error-decoder.html?invariant=124?args[]=foo&args[]=bar
`
- After: ` https://react.dev/errors/124?args[]=foo&args[]=bar`


Requires: reactjs/react.dev#6214

---------

Co-authored-by: Jan Kassens <jkassens@meta.com>
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.

[Beta] Migrating Error Decoder
3 participants