-
Notifications
You must be signed in to change notification settings - Fork 8
[rum-privacy] add sourcemaps and telemetry #187
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
base: master
Are you sure you want to change the base?
Conversation
eaaf2b5
to
ecceaf7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I posted a few nits below:
packages/tests/src/e2e/privacyPluginTypescript/privacyPluginTypescript.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Started a review, will continue later on.
pluginOptions.module === 'cjs' ? './privacy-helpers.js' : './privacy-helpers.mjs', | ||
); | ||
|
||
const privacyHelpersModuleId = pluginOptions.helpersModule ?? PRIVACY_HELPERS_MODULE_ID; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the ?? ...
part?
We don't do it in buildTransformOptions()
.
packages/plugins/rum/src/sdk.ts
Outdated
if (sdkOpts.disabled) { | ||
return ''; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really necessary?
expect(regex.test('.preval.js')).toBe(true); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be done simply using createFilter
and a list of tests cases:
import { createFilter } from '@rollup/pluginutils';
describe('Privacy options', () => {
let filter;
const testCases = [
{ description: 'exclude .preval files', path: 'some-path', expected: false },
// [...]
];
beforeAll(() => {
const pluginOptions = { ...defaultPluginOptions, rum: { privacy: {} } };
const { include, exclude } = validateOptions(pluginOptions, mockLogger);
filter = createFilter(include, exclude);
});
test.each(testCases)('Should $description', ({ path, expected }) => {
expect(filter(path)).toBe(expected);
});
});
packages/plugins/rum/src/validate.ts
Outdated
if (!validatedOptions.sdk || validatedOptions.sdk.disabled) { | ||
return toReturn; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this if
as an early out, I think you can remove the one right below, if (validatedOptions.sdk)
.
ff4c50b
to
cdbd040
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of the addition to the verifyProjectBuild
.
It's very complex, I'll have a look, but I think this can be simplified.
packages/plugins/rum/src/types.ts
Outdated
| 'trackUserInteractions' | ||
| 'trackViewsManually' | ||
> | ||
>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these type changes still necessary?
const buildProject = async ( | ||
bundler: BundlerFullName, | ||
cwd: string, | ||
pluginConfigOverride?: Options, | ||
buildConfigOverride?: BundlerConfig, | ||
) => { | ||
const plugin = allPlugins[bundler](pluginConfigOverride || fullConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const buildProject = async ( | |
bundler: BundlerFullName, | |
cwd: string, | |
pluginConfigOverride?: Options, | |
buildConfigOverride?: BundlerConfig, | |
) => { | |
const plugin = allPlugins[bundler](pluginConfigOverride || fullConfig); | |
const buildProject = async ( | |
bundler: BundlerFullName, | |
cwd: string, | |
pluginConfig?: Options = fullConfig, | |
buildConfigOverride?: BundlerConfig, | |
) => { | |
const plugin = allPlugins[bundler](pluginConfig); |
|
||
// Get the entry for this specific bundler | ||
const bundlerEntry = buildConfigOverride?.entry?.[bundler] || './index.js'; | ||
|
||
// Handle TypeScript compilation for each bundler | ||
const additionalPlugins = [...(buildConfigOverride?.plugins || [])]; | ||
|
||
// Check if any entry is a TypeScript file | ||
const hasTypeScriptEntries = Object.values(buildConfigOverride?.entry || {}).some((entry) => | ||
entry.endsWith('.ts'), | ||
); | ||
|
||
if (hasTypeScriptEntries) { | ||
if (bundler === 'rollup' || bundler === 'vite') { | ||
// Use @rollup/plugin-typescript for Rollup and Vite | ||
additionalPlugins.push( | ||
typescript({ | ||
tsconfig: path.resolve(cwd, 'tsconfig.json'), | ||
}), | ||
); | ||
} | ||
// ESBuild has built-in TypeScript support, no additional plugins needed | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't this logic live at the test level?
As-in, if your test requires to override the build's configuration, it should also handle this part as well (adding the ts plugin to the build).
Can you also give me more details on why you need this @rollup/plugin-typescript
plugin exactly and not re-use the esbuild
plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did try this, but it requires changing the params for verifyProjectBuild
even more, as we need to be able to accept different plugins for different bundlers. We are using @rollup/plugin-typescript
to isolate issues of generated sourcemaps and avoid having two bundlers for the same build, per discussions in slack previously.
packages/tools/src/bundlers.ts
Outdated
// Add TypeScript support for webpack/rspack | ||
if (hasTypeScriptEntries) { | ||
baseConfig.resolve = { | ||
extensions: ['.tsx', '.ts', '.js'], | ||
}; | ||
baseConfig.module = { | ||
rules: [{ test: /\.([cm]?ts|tsx)$/, loader: 'ts-loader' }], | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this even need to be conditional?
Don't we always want to use ts-loader
for any ts
file we encounter? I don't think we need hasTypeScriptEntries
.
What and why?
Improve the privacy plugin and update to the latest
js-instrument-wasm
library.We want to add sourcemaps and telemetry to our privacy plugin.
How?
Change
enforce: pre
topost
for privacy pluginMake sdk injection plugin optional when opt-in for rum plugin
Add
Typescript
end to end testing and supportUpdate transform options to make sourcemaps working
Add logs for observability