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

feat(browser): Export Replay integration from Browser SDK #6508

Merged
merged 7 commits into from
Dec 15, 2022
Merged

feat(browser): Export Replay integration from Browser SDK #6508

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9aeb63e
feat(browser): Export `Replay` integration from Browser SDK
Lms24 Dec 9, 2022
8c3b97e
remove unnecessary plugin sort order modification
Lms24 Dec 13, 2022
56f3932
skip files other than browser index
Lms24 Dec 13, 2022
92661a6
Revert "remove unnecessary plugin sort order modification"
Lms24 Dec 13, 2022
9ef6d3a
remove global regex flag
Lms24 Dec 14, 2022
10ff1a7
add plugin order comment
Lms24 Dec 15, 2022
8552e6b
use `import type` for `BrowserClientReplayOptions`
Lms24 Dec 15, 2022
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
Prev Previous commit
Next Next commit
remove global regex flag
  • Loading branch information
@Lms24
Lms24 committed Dec 14, 2022
commit 9ef6d3a5fdb998bd1894c7a50f82aa011e648143
4 changes: 2 additions & 2 deletions rollup/plugins/bundlePlugins.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ export function makeTSPlugin(jsVersion) {
* If we need to add more such guards in the future, we might want to refactor this into a more generic plugin.
lforst marked this conversation as resolved.
Show resolved Hide resolved
*/
export function makeExcludeReplayPlugin() {
const replacementRegex = /\/\/ __ROLLUP_EXCLUDE_FROM_BUNDLES_BEGIN__(.|\n)*__ROLLUP_EXCLUDE_FROM_BUNDLES_END__/gm;
const replacementRegex = /\/\/ __ROLLUP_EXCLUDE_FROM_BUNDLES_BEGIN__(.|\n)*__ROLLUP_EXCLUDE_FROM_BUNDLES_END__/m;
const browserIndexFilePath = path.resolve(__dirname, '../../packages/browser/src/index.ts');
Copy link
Member

Choose a reason for hiding this comment

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

m: Just a tiny bit worried that if we move this file (or less likely but worse, the place of the guard) that this is gonna break. Thinking can we somehow test for these cases? Or make this more robust.

Honestly, I can't come up with anything obvious that wouldn't be super complicated so I am totally fine with just leaving it as is.

Copy link
Member

Choose a reason for hiding this comment

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

IMHO the best way to check this is via size checks. What about actually decreasing the allowed size limits to reasonable amounts? E.g. currently we have stuff like 100kb in there for browser. What if we just set this to like "current size + 10kb"? Then tests would actually fail when something becomes too large?

Copy link
Member Author

@Lms24 Lms24 Dec 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Great idea with size-check! I can lower the limits as this should be a good indicator when something goes wrong with the replay export removal.

In terms of this being brittle, yes I agree but then again, we're talking about the main index.ts file which IMO is unlikely to change location or name. Obviously, moving the guard would still be a problem but size-check would let us know about this (fwiw, even without limit adjustments as it failed while I still had a bug in this PR).

If we decide to make this plugin more general at some point, we could pass a "transform files allowlist" or something similar to make changing file entries more easy but for now I'd vote to keep the plugin as is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Decided to pull this out into its own PR so we have a better history as to why we made this change: #6543


const plugin = {
Expand All @@ -185,7 +185,7 @@ export function makeExcludeReplayPlugin() {
}

const ms = new MagicString(code);
const transformedCode = ms.replace(new RegExp(replacementRegex), '');
const transformedCode = ms.replace(replacementRegex, '');
return {
code: transformedCode.toString(),
map: transformedCode.generateMap({ hires: true }),
Expand Down