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.

Sign up for GitHub

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

Jump to bottom

ref: Optimize build configs and builds #480

Merged
merged 3 commits into from
Aug 9, 2024
Merged

ref: Optimize build configs and builds #480

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b2e6657
ref: Optimize build configs and builds
BYK Aug 7, 2024
1e6b3ca
revert unrelated change
BYK Aug 7, 2024
4030dee
Merge branch 'main' into byk/ref/optimize-builds
BYK Aug 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions e2e-tests/nextjs/sentry.client.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,7 @@ Sentry.init({
],
});

if (process.env.NODE_ENV === 'development') {
Spotlight.init({
debug: true,
integrations: [customIntegration()],
});
}
Spotlight.init({
Copy link
Member

Choose a reason for hiding this comment

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

I'm aware that this is just an e2e test app but is this how we want users to use Spotlight in the future?
I'm probably missing something but if this guard is removed, spotlight overlay code ends up in the production build of the NextJS application. Currently we document spotlight setup with this guard in place so that users don't accidentally include it in their prod bundles.

The reason is that bundlers are smart enough to remove code within the process.env.NODE_ENV check if the condition evaluates to false at build time.

debug: true,
integrations: [customIntegration()],
});
2 changes: 1 addition & 1 deletion e2e-tests/nextjs/sentry.edge.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,5 @@ Sentry.init({

// Setting this option to true will print useful information to the console while you're setting up Sentry.
debug: false,
spotlight: process.env.NODE_ENV === 'development',
spotlight: true,
Copy link
Member

Choose a reason for hiding this comment

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

same question generally here: If this is always true, the integration will also try to send events to the side car in production. Is this what we want?

Copy link
Member Author

Choose a reason for hiding this comment

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

I never considered these test codes as representative examples for usage (as we also set debug: true on some places). I removed the conditionals mostly to avoid any confusing test issues in case we mess with NODE_ENV value for some reason (builds, installing dependencies etc).

For representative code examples, docs should be the main source of truth and yes ideally, Spotlight should not be included in production code but if one wants to do it, we do have a warning somewhere in the code :)

Copy link
Member

Choose a reason for hiding this comment

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

Ok I see, that makes sense! Fwiw, at least when I worked on Spotlight the e2e tests were quite messy and probably didn't always reflect what we instruct users to do. I think generally, it's good to stay close to how we instruct the setup but honestly, no strong feelings for this one here.

Just wanted to make sure we're aware of the bundling/tree-shaking checks around prod vs dev.

});
2 changes: 1 addition & 1 deletion e2e-tests/nextjs/sentry.server.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,5 @@ Sentry.init({

// Setting this option to true will print useful information to the console while you're setting up Sentry.
debug: false,
spotlight: process.env.NODE_ENV === 'development',
spotlight: true,
});
4 changes: 1 addition & 3 deletions packages/overlay/vite.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,7 @@ export default defineConfig({
removeReactDevToolsMessagePlugin(),
],
define: {
'process.env.NODE_ENV': "'development'",
'process.env.JEST_WORKER_ID': 1,
process: {},
'process.env.NODE_ENV': "'production'",
},
resolve: {
alias: {
Expand Down
3 changes: 1 addition & 2 deletions packages/overlay/vitest.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/// <reference types="vitest" />

// import tsconfigPaths from "vite-tsconfig-paths";
import { resolve } from 'path';
import { resolve } from 'node:path';
import { defineConfig } from 'vitest/config';

export default defineConfig({
Expand All @@ -12,7 +12,6 @@ export default defineConfig({
reporter: ['json'],
},
globals: true,
// setupFiles: ["./src/test/setup-test-env.ts"],
include: ['./test/**/*.{test,spec}.{js,mjs,cjs,ts,mts,cts,jsx,tsx}'],
watchExclude: ['.*\\/node_modules\\/.*', '.*\\/dist\\/.*'],
},
Expand Down
5 changes: 0 additions & 5 deletions packages/sidecar/vite.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ const dependencies = Object.keys({
...packageJson.devDependencies,
});

const noExternal = process.env.NODE_ENV === 'production' ? dependencies : [];

export default defineConfig({
build: {
ssr: './src/main.ts',
Expand All @@ -19,7 +17,4 @@ export default defineConfig({
external: [...dependencies, ...builtinModules.map(x => `node:${x}`)],
},
},
ssr: {
noExternal,
},
});
2 changes: 1 addition & 1 deletion packages/spotlight/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@

This package combines the Overlay from `@spotlightjs/overlay` and the sidecar from `@spotlightjs/sidecar`.

This means, you only need to install one package and you get everything required for spotlight. Nice!
This means, you only need to install one package and you get everything required for Spotlight. Neat!
2 changes: 0 additions & 2 deletions packages/spotlight/vite.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ export default defineConfig({
sidecar: resolve(__dirname, 'src/sidecar.ts'),
'vite-plugin': resolve(__dirname, 'src/vite-plugin.ts'),
},
// the proper extensions will be added
// fileName: 'sentry-spotlight',
},
rollupOptions: {
external: [...dependencies, ...builtinModules.map(x => `node:${x}`)],
Expand Down
2 changes: 1 addition & 1 deletion packages/spotlight/vite.overlay.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { resolve } from 'path';
import { resolve } from 'node:path';
import { defineConfig } from 'vite';

export default defineConfig({
Expand Down
Loading