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(nextjs): Wrap server-side getInitialProps #5546

Merged
merged 25 commits into from
Aug 11, 2022

Conversation

lforst
Copy link
Member

@lforst lforst commented Aug 9, 2022

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:

  • Write JSDoc
  • Write meaningful comments for what the hell hasDefaultExport and getExportIdentifiers are doing
  • Replace patterns like declarationNode.id.type === 'ObjectPattern' with ObjectPattern.check(declarationNode)
  • Check if there's a tool to find all the exports from a file
  • Find a good argument for Templates or not. If no argument is found we stick with what we have and we might clean it up in the future.
  • Check if you can parse JS files with TS parser

@lforst lforst mentioned this pull request Aug 9, 2022
43 tasks
Copy link
Member

@lobsterkatie lobsterkatie left a comment

Choose a reason for hiding this comment

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

I think the overall idea here (proxying) is interesting, and I'm impressed by all of the AST edge-case-handling!

I left a bunch of comments/questions, but probably half of them just have to do with naming and docstrings and stuff.

I have a few other overall questions:

  • Is there any danger that by recursing all the way into the array/object combos that we'll end up with a name which isn't actually top-level?

  • As impressive as all of the AST-wrangling is (and, mea culpa, I started it), it's complicated enough that I keep wondering if there isn't a way to make someone else (node, webpack, etc) do the exports-finding for us. I have a crackpot idea or two here. Let's chat offline.

  • On the one hand, I'll admit that the templates seem like overkill. On the other hand, given that we're already using them, I feel like maybe it'd be nice to be consistent on that score (if for no other reason than that consistency makes it easier to understand for future readers of our code). Are you neutral or opposed on the template score?

  • If we do go with templates across the board, let's move the "is it a function" check into the template, again for consistency.

  • I like the tests! I should probably do the same for my stuff.

packages/nextjs/src/config/loaders/ast.ts Outdated Show resolved Hide resolved
packages/nextjs/src/config/loaders/ast.ts Show resolved Hide resolved
packages/nextjs/src/config/loaders/ast.ts Outdated Show resolved Hide resolved
packages/nextjs/src/config/loaders/ast.ts Outdated Show resolved Hide resolved
packages/nextjs/src/config/loaders/ast.ts Outdated Show resolved Hide resolved
packages/nextjs/src/config/loaders/dataFetchersLoader.ts Outdated Show resolved Hide resolved
packages/nextjs/src/config/loaders/dataFetchersLoader.ts Outdated Show resolved Hide resolved
packages/nextjs/src/config/webpack.ts Outdated Show resolved Hide resolved
packages/nextjs/test/config/ast.test.ts Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Aug 10, 2022

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

@lforst
Copy link
Member Author

lforst commented Aug 10, 2022

@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: aa5b915

@lforst lforst requested a review from lobsterkatie August 10, 2022 16:02
Copy link
Member

@lobsterkatie lobsterkatie left a comment

Choose a reason for hiding this comment

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

This looks good! Thanks for hanging in with my million and one questions/suggestions.

Following up on the points we discussed this morning:

In the meantime, let's ship this!

@lforst lforst marked this pull request as ready for review August 11, 2022 08:16
@lforst lforst enabled auto-merge (squash) August 11, 2022 08:17
@lforst lforst merged commit 2e07b78 into master Aug 11, 2022
@lforst lforst deleted the lforst-wrap-get-initial-props-with-proxy branch August 11, 2022 08:28
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.

2 participants