fix(nextjs): Match loader files exactly #6013
Conversation
| `^${escapeStringForRegex(projectDir)}${ | ||
| shouldIncludeSrcDirectory ? '/src' : '' | ||
| }/pages/.*\\.(${pageExtensionRegex})`, | ||
| }/pages/.*\\.(${pageExtensionRegex})$`, |
There was a problem hiding this comment.
I have to admit this feels a bit risky because of file path shenanigans (windows paths?). I am open to other suggestions.
Making it an exact match should be fine in combination with our query-string import approach because we don't want to process these files anyways. Maybe we can even remove the exit condition (if (this.resourceQuery.includes('__sentry_wrapped__')) return userCode;)
There was a problem hiding this comment.
We should have two regex - one for windows and one for Unix if this one does not support windows imo
There was a problem hiding this comment.
Ugh, right, I always forget about windows path separators. Good call, @AbhiPrasad.
I don't think we need to do it in two regexes, though. We actually already know the pages directory, so I claim we can do this:
There was a problem hiding this comment.
Ah perfect, let’s do that then!
There was a problem hiding this comment.
Katie that change gets rid of the solution for the
srcfolder problematic: #5978
If their pages folder is in a src folder, that should be included in pagesDir, no?
There was a problem hiding this comment.
But we shouldn't include both! It's either pages or src/pages.
There was a problem hiding this comment.
No, I mean: If a user has their pages in /pages, then that will be reflected in the value of pagesDir. If a user has their pages in src/pages, that will also be reflected in the value of pagesDir. Either way, we don't need to recalculate the value.
If I do
console.log(pagesDir);
console.log(pagesDirectory);depending on whether or not I'm using a src directory I get either
/Users/Katie/Documents/Sentry/test-apps/next-js-test/src/pages
/Users/Katie/Documents/Sentry/test-apps/next-js-test/src/pages
or
/Users/Katie/Documents/Sentry/test-apps/next-js-test/pages
/Users/Katie/Documents/Sentry/test-apps/next-js-test/pages
Update: Opened #6044 to remove the extra calculation.
There was a problem hiding this comment.
Oh sorry got confused and didn't realize that pagesDir was a thing. I'll approve that PR!
size-limit report 📦
|
| @@ -86,18 +86,17 @@ export function constructWebpackConfigFunction( | |||
| // if `pages` is present in the root directory" | |||
| // - https://nextjs.org/docs/advanced-features/src-directory | |||
| const shouldIncludeSrcDirectory = !fs.existsSync(path.resolve(projectDir, 'pages')); | |||
| const pagesDirectory = shouldIncludeSrcDirectory // We're intentionally not including slashes in the paths because we wanne let node do the path resolution for Windows | |||
There was a problem hiding this comment.
Less than l: I would remove the comment, don’t think that context is needed.
| const pagesDirectory = shouldIncludeSrcDirectory // We're intentionally not including slashes in the paths because we wanne let node do the path resolution for Windows | |
| const pagesDirectory = shouldIncludeSrcDirectory |
There was a problem hiding this comment.
I'm gonna leave it just so there's some awareness around windows in that file. That was the original reason I even left a comment.
Probably resolves #6012
I couldn't really reproduce #6012 but I noticed that we're applying our proxy loader to JSON files because our loader regex matches on
.jsand.jshappens to be a substring of.json.In this PR we attempt to fix this by making the loader RegExp an exact match.