Skip to content

Conversation

@onurtemizkan
Copy link
Collaborator

Fixes: #3538

next-pwa explicitly inserts its registration script to main.js entry. In this case, Sentry should also be injected into it.

@github-actions
Copy link
Contributor

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 21.01 KB (-0.01% 🔽)
@sentry/browser - Webpack 21.89 KB (0%)
@sentry/react - Webpack 21.92 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 28.4 KB (-0.01% 🔽)

@lobsterkatie
Copy link
Member

Hmmm - you've tested this with next-pwa and before the change it's broken and after the change it's not?

I ask because the main.js entry point is deprecated, and next just moves anything there over into main. So next-pwa may be sticking stuff in main.js, but we (and next itself) use main, and eventually everything ends up in main anyway.

@onurtemizkan
Copy link
Collaborator Author

Yes, tested it before and after the change. I think the problem might be that next probably moves main.js to main after we do injections, so the pwa registration script ends up untouched.

Debugger output of where we run _injectFile:

Screenshot 2021-06-17 at 15 05 10

@lobsterkatie
Copy link
Member

Ah - I figured it out. In cases where the main.js entry point has anything in it, Next does this:

const originalFile = clientEntries[
  CLIENT_STATIC_FILES_RUNTIME_MAIN
] as string
entry[CLIENT_STATIC_FILES_RUNTIME_MAIN] = [
  ...entry['main.js'],
  originalFile,
]

I was only really paying attention to what happens to the main.js stuff and therefore I hadn't read closely enough to notice
a) originalFile comes from clientEntries (which is from before we add anything), not from entry (which includes the injected Sentry file), and
b) that it's originalFile, not ...originalFiles.

IMHO this is a bug in Next itself, but in the meantime...

I see how your fix fixes it, but I think it can be done in a slightly simpler way. You're testing for 'main.js' in newEntryProperty), but that will always evaluate to true. So newEntryProperty will always end up with main: ['./node_modules/next/dist/client/next.js', './sentry.client.config.js'] and "main.js": ['./sentry.client.config.js']. The fact that there's something in the main.js array means next will always fall into that conditional (regardless of any other plugin), ditch our changes to main, and keep our changes (and any other plugin's) changes to main.js. In the end it doesn't matter that we're adding the same file in two places, because only one of them sticks around.

That said, if we know that, we can avoid adding it twice in the first place. So my first thought was to only add it to main.js (and do that always), and let Next copy it over into main itself. The only problem with that is that then we'd regress on this, since Next moves main.js stuff in front of the file from main. So I think we have to move everything from main.js over into main ourselves (and then empty out or delete the main.js entry).

I'm actually doing a big refactoring of the config code which will land later today, and then a few other config changes. Would you be offended if we closed this and I just rolled this up into that work?

@onurtemizkan
Copy link
Collaborator Author

Thanks for the explanation.

Would you be offended if we closed this and I just rolled this up into that work?

Not at all 😄

@onurtemizkan onurtemizkan deleted the fix/next-pwa-entry branch June 17, 2021 16:24
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.

Next.js plugin doesn't work with next-pwa plugin #213

3 participants