ref(nextjs): Wrap server-side data-fetching methods during build #5503
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
lobsterkatie
force-pushed
the
kmclb-nextjs-wrap-data-fetching-methods
branch
from
f588a2b to
f368c40
Compare
size-limit report 📦
|
This is a number of boring structural changes pulled out of #5503 in order to make it easier to review. Included changes: - Create folders for loaders and templates, and move the existing loader and template into their respective new folders. - Convert the `LoaderThis` type into a generic, so that each loader can specify its own options but share everything specified by webpack. - Since we are only going to support wrapping for pages written in ESM, make sure that templates are ESM, even if the CJS version of the SDK is being used. This required creating a build script, which in turn required updating our ESLint config. - Add/clarify comments in existing loader files.
lobsterkatie
force-pushed
the
kmclb-nextjs-wrap-data-fetching-methods
branch
3 times, most recently
from
05cb4b5 to
130169d
Compare
packages/nextjs/src/config/templates/dataFetchersLoaderTemplate.ts
Outdated
Show resolved
Hide resolved
lobsterkatie
force-pushed
the
kmclb-nextjs-wrap-data-fetching-methods
branch
2 times, most recently
from
cd3e26b to
711fa2f
Compare
|
ABSOLUTE mind boggle: I tried to create a simple page in CJS and it just worked. I think you can just copy the following code into any next page and it should compile and run totally fine. The SDK doesn't seem to be working however. 🤔 const { default: Head } = require("next/head");
const { useState } = require("react");
const Sentry = require("@sentry/nextjs");
const Home = () => {
const [shouldThrow, setShouldThrow] = useState(false);
if (shouldThrow) {
throw new Error("This is some render error!");
}
return (
<div>
<Head>
<title>Create Next App</title>
<meta name="description" content="Generated by create next app" />
<link rel="icon" href="/favicon.ico" />
</Head>
<h1>Next.js Test Page</h1>
<form>
<input
type="button"
value="trigger onClick error"
onClick={() => {
try {
throw new Error("This is some test onClick error!");
} catch (err) {
Sentry.captureException(err);
}
}}
/>
<br />
<input
type="button"
value="trigger render error"
onClick={() => {
setShouldThrow(true);
}}
/>
</form>
</div>
);
};
module.exports = {
default: Home,
}; |
There was a problem hiding this comment.
Ok - so this does have the chance to break folks pretty badly as we are mucking with ASTs.
What do you think about this?
a) We gate this functionality behind an SDK option
b) We update the Sentry onboarding wizard/docs to enable this option by default - so only new users of the SDK will encounter possible issues with this for the most part
c) After a week or 2 passes, we enable the option by default
Do we care about this risk that much? Maybe it's fine to just 
Honestly, my instinct was the same (to make it opt-in at first), but Vladan expressed that he felt that was being too cautious and we didn't need to bother. Do you still feel that way @vladanpaunovic? The more I dig into the different cases I'm needing to cover, the more I'm leaning back towards thinking it should start as experimental. Either way, I think we should make a flag, so that users can either opt in if they choose or opt out if we choose and end up breaking things. (On that topic @lforst made a good point, though: We should figure out if doing this could break people only at build time - not great, but at least they know right away - or if it could also break in such a way that they don't find out until we crash their app (or at least render certain pages non-functional) at runtime - way worse, and something we obviously want to avoid in any way we can.) UPDATE: We discussed this offline and will put this behind a flag for now, so that it's opt-in. |
lobsterkatie
force-pushed
the
kmclb-nextjs-wrap-data-fetching-methods
branch
2 times, most recently
from
dbd8903 to
57f6065
Compare
lobsterkatie
force-pushed
the
kmclb-nextjs-wrap-data-fetching-methods
branch
from
57f6065 to
d9e4118
Compare
Agreed. @lobsterkatie please, create an issue somewhere in our backlog to promote the experimental feature into a stable one once we've gained more confidence. |
|
After upgrading
It is mainly to do with the introduction of Some of I am not suggesting to give up |
@kachkaev Thanks for the heads up! It's actually something I didn't really consider when reviewing. We're currently rethinking what we did in this PR and finding a different approach using |
Ref: Ref: #5505
This adds wrapping of the nextjs server-side data-fetching methods (
getStaticPaths,getStaticProps, andgetServerSideProps) to the webpack config changes we make during app build. Eventually, this will let us create spans for them, use them to improve route parameterization, and trace requests which aren't currently getting traced on Vercel-deployed nextjs apps. For the moment, though, all the wrappers do is call the functions they wrap, in order to separate out the wrapping into its own PR.Notes:
For the moment, in order to allow us to iterate on it without fear of breaking people's apps, this feature is gated behind the
sentry.autoWrapDataFetchersflag innext.config.js.To do the wrapping, we scan each page's AST for references to each of the three functions. If a given function is found, it is renamed (and all references to it similarly adjusted) so that its name starts with one or more underscores (so
getServerSidePropsbecomes_getServerSideProps, for example). This then allows us to create a new, wrapped version of the (now-renamed) function, and export that wrapped version under the original name. (For example, we can then addexport const getServerSideProps = withSentryGSSP(_getServerSideProps)to the page code using the loader, and nextjs will never know the difference.)Parsing of user code into an AST (and subsequent traversal of that AST) is done using
@babel/parserandjscodeshift. (@babel/parseris used directly becausejscodeshiftneither exposes its parsers nor provides an option for specifying a parser by name (at least when used programmatically). Therefore, in order to use a jsx parser for JS pages and a tsx parser for TS pages, we need to supply them ourselves.)Follow-up work: