sentry-javascript icon indicating copy to clipboard operation
sentry-javascript copied to clipboard

feat(nextjs): Wrap server-side getInitialProps

Open lforst opened this issue 3 years ago • 1 comments

This PR wraps server-side getInitialProps. We do this by creating a virtual proxy module for each next.js pages in our dataFetchersLoader webpack loader that re-exports all the exports of the original pages file but with a wrapped version of getInitiialProps.

The reason we went for the virtual proxy instead of AST modifications, is that in order to wrap getInitialProps we have to cover a lot of edge cases of how 1. people might define a default export 2. define getInitialProps on the default export. The proxy solution allows us to generically cover all of those cases with relatively little effort. Another upside is, that we do not have to mess with user code potentially creating runtime bugs - instead, we only mess with imports/exports which only throw at build time.

The AST modification code that wraps getServerSideProps, getStaticProps, and getStaticPaths currently coexists with the proxy module code. If we deem the proxy code good, we might switch the wrapping of those methods to the proxy approach too.

Additional notes:

  • fixed a small bug where not all instances of __ORIG_FUNC__ got replaced
  • renamed all the wrapping functions because the abbreviations gave me a headache
  • "improved" the loaderThis type (also removed the TODO comment saying we should replace it with webpack types - I don't think we should do this since it's super painful to maintain and implement with our current typescript version which is yet too dumb/slow to do good inference)
  • I didn't put the injected code in a template since the gain we would have from that doesn't outweigh the additional code complexity

Draft until TODOs are done:

  • [x] Write JSDoc
  • [ ] Write meaningful comments for what the hell hasDefaultExport and getExportIdentifiers are doing
  • [x] Replace patterns like declarationNode.id.type === 'ObjectPattern' with ObjectPattern.check(declarationNode)

lforst avatar Aug 09 '22 12:08 lforst

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.39 KB (0%)
@sentry/browser - ES5 CDN Bundle (minified) 59.99 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 17.96 KB (+0.03% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 52.88 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 19.73 KB (0%)
@sentry/browser - Webpack (minified) 64.21 KB (0%)
@sentry/react - Webpack (gzipped + minified) 19.75 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 44.16 KB (+0.01% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 25.79 KB (+0.02% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.04 KB (0%)

github-actions[bot] avatar Aug 10 '22 09:08 github-actions[bot]

@lobsterkatie Okay so there was this overarching theme in your comments asking why I prefixed all the function's name with export even though they appear to be generic.

The reason is 🥁... they aren't. The functions only do what they say they're doing when they are inside an export statement. It is entirely possible we encounter ObjectPatterns, ArrayPatterns and RestElements outside of an export statement. In these situations, the functions wouldn't do the right thing though because there are much more edge cases to handle.

On the right-hand side of an assignment you can do stuff like: const asdf = { ...({ a: 1 } || { b: 2 }) }; whereas when you encounter an ObjectPattern on the left-hand side, you can't do that - you're actually super limited in what you are allowed to do.

I added a comment about why you're only allowed to use these functions to extract identifiers only in an export context: aa5b915872d776268bdc71ad6551afde98725561

lforst avatar Aug 10 '22 15:08 lforst