Skip to content

feat(ci): Run yarn-deduplicate in CI - #15119

Merged
lforst merged 16 commits into
getsentry:developfrom
nwalters512:ci-yarn-deduplicate
Mar 17, 2025
Merged

feat(ci): Run yarn-deduplicate in CI#15119
lforst merged 16 commits into
getsentry:developfrom
nwalters512:ci-yarn-deduplicate

Conversation

@nwalters512

Copy link
Copy Markdown
Contributor

This PR introduces a CI step to use yarn-deduplicate to ensure that packages are maximally deduped. This makes version upgrades easier and minimizes the amount of packages that need to be installed by Yarn.

I added this to the lint job, as that seemed the best place for it. Happy to move it elsewhere if desired.

Related to #14968.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@AbhiPrasad 👋 do you have any interest in merging this? I think it'll be a net positive for the project and should make it easier to keep dependencies up-to-date.

@AbhiPrasad

Copy link
Copy Markdown
Contributor

Hey @nwalters512 sorry for the delay! was on vacation and then a bunch of other things came up.

I agree this is a net positive, just need to find some time to set aside to merge it in and update team process stuff for us internally.

@AbhiPrasadAbhiPrasad self-assigned this Mar 4, 2025
@lforstlforst assigned lforst and unassigned AbhiPrasadMar 5, 2025
@lforst
lforst enabled auto-merge (squash) March 10, 2025 11:59
@lforst

Copy link
Copy Markdown
Contributor

E2E tests do not seem to like the deduplication. Will close for now.

@lforstlforst closed this Mar 10, 2025
auto-merge was automatically disabled March 10, 2025 13:40

Pull request was closed

@lforst

Copy link
Copy Markdown
Contributor

It's probably an issue with how we defined deps in our packages but not sure.

@lforst

Copy link
Copy Markdown
Contributor

Nvm, seems to fail on develop too

@lforstlforst reopened this Mar 10, 2025
@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@lforst one potential way to narrow down the issue is to dedupe dependencies one-by-one in separate PRs until we find the one that breaks the E2E tests (currently seems to just be nextjs-13). I'd be happy to open those PRs. Unfortunately, this is difficult for me to test localls, as I can't even get the E2E tests to pass locally on the develop branch.

@lforst

Copy link
Copy Markdown
Contributor

@nwalters512 I'd love to merge this but I can't justify spending time on it. None of this is user facing unfortunately.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@lforst understood, hence my offer to continue driving this forward myself. If you don't even want to spend time reviewing other dependency bump/dedupe PRs, I'd understand that too though.

@lforst

Copy link
Copy Markdown
Contributor

@nwalters512 Oh if you manage to make the tests pass please go ahead 😄 I they pass we can merge. I just won't spend time on it any longer I think.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I'll see what I can do! The fact that just changing dependencies within their semver-compatible versions can make tests fail is a bit of a red flag, so there's probably some interesting deeper issue here.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

First round of this is #15639, all tests pass after deduping all @babel dependencies.

Lms24 pushed a commit that referenced this pull request Mar 13, 2025
Splitting this out of #15119 in the hope that gradually deduping things
will help identify which dependency is causing the Next E2E tests to
fail in that PR.
@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I was able to get the nextjs-13 tests passing locally for me on develop. They start failing with a timeout as soon as rollup is deduplicated:

 1) [chromium] › tests/client/pages-dir-pageload.test.ts:50:5 › should create a pageload transaction with correct name when an error occurs in getServerSideProps Test timeout of 30000ms exceeded.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I've narrowed it down to rollup@4.34.0. Things work with v4.33.0, and fail with v4.34.0, the next version that was published.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I believe Rollup is stripping properties out of an object when it shouldn't be.

Here's the relevant code:

constgetInitialPropsWrappers: Record<string,any>={
'/_app': Sentry.wrapAppGetInitialPropsWithSentry,
'/_document': Sentry.wrapDocumentGetInitialPropsWithSentry,
'/_error': Sentry.wrapErrorGetInitialPropsWithSentry,
};

Here's how it looks when things work (old version of Rollup):

constgetInitialPropsWrappers={'/_app': Sentry.wrapAppGetInitialPropsWithSentry,'/_document': Sentry.wrapDocumentGetInitialPropsWithSentry,'/_error': Sentry.wrapErrorGetInitialPropsWithSentry,};

And when things break (newest version of Rollup):

constgetInitialPropsWrappers={};

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I opened #15651 to independently upgrade to the latest version of Rollup and fix the bug that it caused in the @sentry/nextjs package.

@lforst
lforst merged commit 16e40f4 into getsentry:developMar 17, 2025
@nwalters512
nwalters512 deleted the ci-yarn-deduplicate branch March 17, 2025 15:54
Sign up for freeto 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.

3 participants

@nwalters512@AbhiPrasad@lforst
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
feat(ci): Run yarn-deduplicate in CI by nwalters512 · Pull Request #15119 · getsentry/sentry-javascript · GitHub
Skip to content

feat(ci): Run yarn-deduplicate in CI - #15119

Merged
lforst merged 16 commits into
getsentry:developfrom
nwalters512:ci-yarn-deduplicate
Mar 17, 2025
Merged

feat(ci): Run yarn-deduplicate in CI#15119
lforst merged 16 commits into
getsentry:developfrom
nwalters512:ci-yarn-deduplicate

Conversation

@nwalters512

Copy link
Copy Markdown
Contributor

This PR introduces a CI step to use yarn-deduplicate to ensure that packages are maximally deduped. This makes version upgrades easier and minimizes the amount of packages that need to be installed by Yarn.

I added this to the lint job, as that seemed the best place for it. Happy to move it elsewhere if desired.

Related to #14968.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@AbhiPrasad 👋 do you have any interest in merging this? I think it'll be a net positive for the project and should make it easier to keep dependencies up-to-date.

@AbhiPrasad

Copy link
Copy Markdown
Contributor

Hey @nwalters512 sorry for the delay! was on vacation and then a bunch of other things came up.

I agree this is a net positive, just need to find some time to set aside to merge it in and update team process stuff for us internally.

@AbhiPrasadAbhiPrasad self-assigned this Mar 4, 2025
@lforstlforst assigned lforst and unassigned AbhiPrasadMar 5, 2025
@lforst
lforst enabled auto-merge (squash) March 10, 2025 11:59
@lforst

Copy link
Copy Markdown
Contributor

E2E tests do not seem to like the deduplication. Will close for now.

@lforstlforst closed this Mar 10, 2025
auto-merge was automatically disabled March 10, 2025 13:40

Pull request was closed

@lforst

Copy link
Copy Markdown
Contributor

It's probably an issue with how we defined deps in our packages but not sure.

@lforst

Copy link
Copy Markdown
Contributor

Nvm, seems to fail on develop too

@lforstlforst reopened this Mar 10, 2025
@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@lforst one potential way to narrow down the issue is to dedupe dependencies one-by-one in separate PRs until we find the one that breaks the E2E tests (currently seems to just be nextjs-13). I'd be happy to open those PRs. Unfortunately, this is difficult for me to test localls, as I can't even get the E2E tests to pass locally on the develop branch.

@lforst

Copy link
Copy Markdown
Contributor

@nwalters512 I'd love to merge this but I can't justify spending time on it. None of this is user facing unfortunately.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@lforst understood, hence my offer to continue driving this forward myself. If you don't even want to spend time reviewing other dependency bump/dedupe PRs, I'd understand that too though.

@lforst

Copy link
Copy Markdown
Contributor

@nwalters512 Oh if you manage to make the tests pass please go ahead 😄 I they pass we can merge. I just won't spend time on it any longer I think.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I'll see what I can do! The fact that just changing dependencies within their semver-compatible versions can make tests fail is a bit of a red flag, so there's probably some interesting deeper issue here.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

First round of this is #15639, all tests pass after deduping all @babel dependencies.

Lms24 pushed a commit that referenced this pull request Mar 13, 2025
Splitting this out of #15119 in the hope that gradually deduping things
will help identify which dependency is causing the Next E2E tests to
fail in that PR.
@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I was able to get the nextjs-13 tests passing locally for me on develop. They start failing with a timeout as soon as rollup is deduplicated:

 1) [chromium] › tests/client/pages-dir-pageload.test.ts:50:5 › should create a pageload transaction with correct name when an error occurs in getServerSideProps Test timeout of 30000ms exceeded.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I've narrowed it down to rollup@4.34.0. Things work with v4.33.0, and fail with v4.34.0, the next version that was published.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I believe Rollup is stripping properties out of an object when it shouldn't be.

Here's the relevant code:

constgetInitialPropsWrappers: Record<string,any>={
'/_app': Sentry.wrapAppGetInitialPropsWithSentry,
'/_document': Sentry.wrapDocumentGetInitialPropsWithSentry,
'/_error': Sentry.wrapErrorGetInitialPropsWithSentry,
};

Here's how it looks when things work (old version of Rollup):

constgetInitialPropsWrappers={'/_app': Sentry.wrapAppGetInitialPropsWithSentry,'/_document': Sentry.wrapDocumentGetInitialPropsWithSentry,'/_error': Sentry.wrapErrorGetInitialPropsWithSentry,};

And when things break (newest version of Rollup):

constgetInitialPropsWrappers={};

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I opened #15651 to independently upgrade to the latest version of Rollup and fix the bug that it caused in the @sentry/nextjs package.

@lforst
lforst merged commit 16e40f4 into getsentry:developMar 17, 2025
@nwalters512
nwalters512 deleted the ci-yarn-deduplicate branch March 17, 2025 15:54
Sign up for freeto 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.

3 participants

@nwalters512@AbhiPrasad@lforst
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' feat(ci): Run yarn-deduplicate in CI by nwalters512 · Pull Request #15119 · getsentry/sentry-javascript · GitHub
Skip to content

feat(ci): Run yarn-deduplicate in CI - #15119

Merged
lforst merged 16 commits into
getsentry:developfrom
nwalters512:ci-yarn-deduplicate
Mar 17, 2025
Merged

feat(ci): Run yarn-deduplicate in CI#15119
lforst merged 16 commits into
getsentry:developfrom
nwalters512:ci-yarn-deduplicate

Conversation

@nwalters512

Copy link
Copy Markdown
Contributor

This PR introduces a CI step to use yarn-deduplicate to ensure that packages are maximally deduped. This makes version upgrades easier and minimizes the amount of packages that need to be installed by Yarn.

I added this to the lint job, as that seemed the best place for it. Happy to move it elsewhere if desired.

Related to #14968.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@AbhiPrasad 👋 do you have any interest in merging this? I think it'll be a net positive for the project and should make it easier to keep dependencies up-to-date.

@AbhiPrasad

Copy link
Copy Markdown
Contributor

Hey @nwalters512 sorry for the delay! was on vacation and then a bunch of other things came up.

I agree this is a net positive, just need to find some time to set aside to merge it in and update team process stuff for us internally.

@AbhiPrasadAbhiPrasad self-assigned this Mar 4, 2025
@lforstlforst assigned lforst and unassigned AbhiPrasadMar 5, 2025
@lforst
lforst enabled auto-merge (squash) March 10, 2025 11:59
@lforst

Copy link
Copy Markdown
Contributor

E2E tests do not seem to like the deduplication. Will close for now.

@lforstlforst closed this Mar 10, 2025
auto-merge was automatically disabled March 10, 2025 13:40

Pull request was closed

@lforst

Copy link
Copy Markdown
Contributor

It's probably an issue with how we defined deps in our packages but not sure.

@lforst

Copy link
Copy Markdown
Contributor

Nvm, seems to fail on develop too

@lforstlforst reopened this Mar 10, 2025
@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@lforst one potential way to narrow down the issue is to dedupe dependencies one-by-one in separate PRs until we find the one that breaks the E2E tests (currently seems to just be nextjs-13). I'd be happy to open those PRs. Unfortunately, this is difficult for me to test localls, as I can't even get the E2E tests to pass locally on the develop branch.

@lforst

Copy link
Copy Markdown
Contributor

@nwalters512 I'd love to merge this but I can't justify spending time on it. None of this is user facing unfortunately.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@lforst understood, hence my offer to continue driving this forward myself. If you don't even want to spend time reviewing other dependency bump/dedupe PRs, I'd understand that too though.

@lforst

Copy link
Copy Markdown
Contributor

@nwalters512 Oh if you manage to make the tests pass please go ahead 😄 I they pass we can merge. I just won't spend time on it any longer I think.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I'll see what I can do! The fact that just changing dependencies within their semver-compatible versions can make tests fail is a bit of a red flag, so there's probably some interesting deeper issue here.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

First round of this is #15639, all tests pass after deduping all @babel dependencies.

Lms24 pushed a commit that referenced this pull request Mar 13, 2025
Splitting this out of #15119 in the hope that gradually deduping things
will help identify which dependency is causing the Next E2E tests to
fail in that PR.
@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I was able to get the nextjs-13 tests passing locally for me on develop. They start failing with a timeout as soon as rollup is deduplicated:

 1) [chromium] › tests/client/pages-dir-pageload.test.ts:50:5 › should create a pageload transaction with correct name when an error occurs in getServerSideProps Test timeout of 30000ms exceeded.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I've narrowed it down to rollup@4.34.0. Things work with v4.33.0, and fail with v4.34.0, the next version that was published.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I believe Rollup is stripping properties out of an object when it shouldn't be.

Here's the relevant code:

constgetInitialPropsWrappers: Record<string,any>={
'/_app': Sentry.wrapAppGetInitialPropsWithSentry,
'/_document': Sentry.wrapDocumentGetInitialPropsWithSentry,
'/_error': Sentry.wrapErrorGetInitialPropsWithSentry,
};

Here's how it looks when things work (old version of Rollup):

constgetInitialPropsWrappers={'/_app': Sentry.wrapAppGetInitialPropsWithSentry,'/_document': Sentry.wrapDocumentGetInitialPropsWithSentry,'/_error': Sentry.wrapErrorGetInitialPropsWithSentry,};

And when things break (newest version of Rollup):

constgetInitialPropsWrappers={};

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I opened #15651 to independently upgrade to the latest version of Rollup and fix the bug that it caused in the @sentry/nextjs package.

@lforst
lforst merged commit 16e40f4 into getsentry:developMar 17, 2025
@nwalters512
nwalters512 deleted the ci-yarn-deduplicate branch March 17, 2025 15:54
Sign up for freeto 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.

3 participants

@nwalters512@AbhiPrasad@lforst
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' feat(ci): Run yarn-deduplicate in CI by nwalters512 · Pull Request #15119 · getsentry/sentry-javascript · GitHub
Skip to content

feat(ci): Run yarn-deduplicate in CI - #15119

Merged
lforst merged 16 commits into
getsentry:developfrom
nwalters512:ci-yarn-deduplicate
Mar 17, 2025
Merged

feat(ci): Run yarn-deduplicate in CI#15119
lforst merged 16 commits into
getsentry:developfrom
nwalters512:ci-yarn-deduplicate

Conversation

@nwalters512

Copy link
Copy Markdown
Contributor

This PR introduces a CI step to use yarn-deduplicate to ensure that packages are maximally deduped. This makes version upgrades easier and minimizes the amount of packages that need to be installed by Yarn.

I added this to the lint job, as that seemed the best place for it. Happy to move it elsewhere if desired.

Related to #14968.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@AbhiPrasad 👋 do you have any interest in merging this? I think it'll be a net positive for the project and should make it easier to keep dependencies up-to-date.

@AbhiPrasad

Copy link
Copy Markdown
Contributor

Hey @nwalters512 sorry for the delay! was on vacation and then a bunch of other things came up.

I agree this is a net positive, just need to find some time to set aside to merge it in and update team process stuff for us internally.

@AbhiPrasadAbhiPrasad self-assigned this Mar 4, 2025
@lforstlforst assigned lforst and unassigned AbhiPrasadMar 5, 2025
@lforst
lforst enabled auto-merge (squash) March 10, 2025 11:59
@lforst

Copy link
Copy Markdown
Contributor

E2E tests do not seem to like the deduplication. Will close for now.

@lforstlforst closed this Mar 10, 2025
auto-merge was automatically disabled March 10, 2025 13:40

Pull request was closed

@lforst

Copy link
Copy Markdown
Contributor

It's probably an issue with how we defined deps in our packages but not sure.

@lforst

Copy link
Copy Markdown
Contributor

Nvm, seems to fail on develop too

@lforstlforst reopened this Mar 10, 2025
@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@lforst one potential way to narrow down the issue is to dedupe dependencies one-by-one in separate PRs until we find the one that breaks the E2E tests (currently seems to just be nextjs-13). I'd be happy to open those PRs. Unfortunately, this is difficult for me to test localls, as I can't even get the E2E tests to pass locally on the develop branch.

@lforst

Copy link
Copy Markdown
Contributor

@nwalters512 I'd love to merge this but I can't justify spending time on it. None of this is user facing unfortunately.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@lforst understood, hence my offer to continue driving this forward myself. If you don't even want to spend time reviewing other dependency bump/dedupe PRs, I'd understand that too though.

@lforst

Copy link
Copy Markdown
Contributor

@nwalters512 Oh if you manage to make the tests pass please go ahead 😄 I they pass we can merge. I just won't spend time on it any longer I think.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I'll see what I can do! The fact that just changing dependencies within their semver-compatible versions can make tests fail is a bit of a red flag, so there's probably some interesting deeper issue here.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

First round of this is #15639, all tests pass after deduping all @babel dependencies.

Lms24 pushed a commit that referenced this pull request Mar 13, 2025
Splitting this out of #15119 in the hope that gradually deduping things
will help identify which dependency is causing the Next E2E tests to
fail in that PR.
@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I was able to get the nextjs-13 tests passing locally for me on develop. They start failing with a timeout as soon as rollup is deduplicated:

 1) [chromium] › tests/client/pages-dir-pageload.test.ts:50:5 › should create a pageload transaction with correct name when an error occurs in getServerSideProps Test timeout of 30000ms exceeded.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I've narrowed it down to rollup@4.34.0. Things work with v4.33.0, and fail with v4.34.0, the next version that was published.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I believe Rollup is stripping properties out of an object when it shouldn't be.

Here's the relevant code:

constgetInitialPropsWrappers: Record<string,any>={
'/_app': Sentry.wrapAppGetInitialPropsWithSentry,
'/_document': Sentry.wrapDocumentGetInitialPropsWithSentry,
'/_error': Sentry.wrapErrorGetInitialPropsWithSentry,
};

Here's how it looks when things work (old version of Rollup):

constgetInitialPropsWrappers={'/_app': Sentry.wrapAppGetInitialPropsWithSentry,'/_document': Sentry.wrapDocumentGetInitialPropsWithSentry,'/_error': Sentry.wrapErrorGetInitialPropsWithSentry,};

And when things break (newest version of Rollup):

constgetInitialPropsWrappers={};

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I opened #15651 to independently upgrade to the latest version of Rollup and fix the bug that it caused in the @sentry/nextjs package.

@lforst
lforst merged commit 16e40f4 into getsentry:developMar 17, 2025
@nwalters512
nwalters512 deleted the ci-yarn-deduplicate branch March 17, 2025 15:54
Sign up for freeto 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.

3 participants

@nwalters512@AbhiPrasad@lforst
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' feat(ci): Run yarn-deduplicate in CI by nwalters512 · Pull Request #15119 · getsentry/sentry-javascript · GitHub
Skip to content

feat(ci): Run yarn-deduplicate in CI - #15119

Merged
lforst merged 16 commits into
getsentry:developfrom
nwalters512:ci-yarn-deduplicate
Mar 17, 2025
Merged

feat(ci): Run yarn-deduplicate in CI#15119
lforst merged 16 commits into
getsentry:developfrom
nwalters512:ci-yarn-deduplicate

Conversation

@nwalters512

Copy link
Copy Markdown
Contributor

This PR introduces a CI step to use yarn-deduplicate to ensure that packages are maximally deduped. This makes version upgrades easier and minimizes the amount of packages that need to be installed by Yarn.

I added this to the lint job, as that seemed the best place for it. Happy to move it elsewhere if desired.

Related to #14968.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@AbhiPrasad 👋 do you have any interest in merging this? I think it'll be a net positive for the project and should make it easier to keep dependencies up-to-date.

@AbhiPrasad

Copy link
Copy Markdown
Contributor

Hey @nwalters512 sorry for the delay! was on vacation and then a bunch of other things came up.

I agree this is a net positive, just need to find some time to set aside to merge it in and update team process stuff for us internally.

@AbhiPrasadAbhiPrasad self-assigned this Mar 4, 2025
@lforstlforst assigned lforst and unassigned AbhiPrasadMar 5, 2025
@lforst
lforst enabled auto-merge (squash) March 10, 2025 11:59
@lforst

Copy link
Copy Markdown
Contributor

E2E tests do not seem to like the deduplication. Will close for now.

@lforstlforst closed this Mar 10, 2025
auto-merge was automatically disabled March 10, 2025 13:40

Pull request was closed

@lforst

Copy link
Copy Markdown
Contributor

It's probably an issue with how we defined deps in our packages but not sure.

@lforst

Copy link
Copy Markdown
Contributor

Nvm, seems to fail on develop too

@lforstlforst reopened this Mar 10, 2025
@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@lforst one potential way to narrow down the issue is to dedupe dependencies one-by-one in separate PRs until we find the one that breaks the E2E tests (currently seems to just be nextjs-13). I'd be happy to open those PRs. Unfortunately, this is difficult for me to test localls, as I can't even get the E2E tests to pass locally on the develop branch.

@lforst

Copy link
Copy Markdown
Contributor

@nwalters512 I'd love to merge this but I can't justify spending time on it. None of this is user facing unfortunately.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@lforst understood, hence my offer to continue driving this forward myself. If you don't even want to spend time reviewing other dependency bump/dedupe PRs, I'd understand that too though.

@lforst

Copy link
Copy Markdown
Contributor

@nwalters512 Oh if you manage to make the tests pass please go ahead 😄 I they pass we can merge. I just won't spend time on it any longer I think.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I'll see what I can do! The fact that just changing dependencies within their semver-compatible versions can make tests fail is a bit of a red flag, so there's probably some interesting deeper issue here.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

First round of this is #15639, all tests pass after deduping all @babel dependencies.

Lms24 pushed a commit that referenced this pull request Mar 13, 2025
Splitting this out of #15119 in the hope that gradually deduping things
will help identify which dependency is causing the Next E2E tests to
fail in that PR.
@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I was able to get the nextjs-13 tests passing locally for me on develop. They start failing with a timeout as soon as rollup is deduplicated:

 1) [chromium] › tests/client/pages-dir-pageload.test.ts:50:5 › should create a pageload transaction with correct name when an error occurs in getServerSideProps Test timeout of 30000ms exceeded.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I've narrowed it down to rollup@4.34.0. Things work with v4.33.0, and fail with v4.34.0, the next version that was published.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I believe Rollup is stripping properties out of an object when it shouldn't be.

Here's the relevant code:

constgetInitialPropsWrappers: Record<string,any>={
'/_app': Sentry.wrapAppGetInitialPropsWithSentry,
'/_document': Sentry.wrapDocumentGetInitialPropsWithSentry,
'/_error': Sentry.wrapErrorGetInitialPropsWithSentry,
};

Here's how it looks when things work (old version of Rollup):

constgetInitialPropsWrappers={'/_app': Sentry.wrapAppGetInitialPropsWithSentry,'/_document': Sentry.wrapDocumentGetInitialPropsWithSentry,'/_error': Sentry.wrapErrorGetInitialPropsWithSentry,};

And when things break (newest version of Rollup):

constgetInitialPropsWrappers={};

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I opened #15651 to independently upgrade to the latest version of Rollup and fix the bug that it caused in the @sentry/nextjs package.

@lforst
lforst merged commit 16e40f4 into getsentry:developMar 17, 2025
@nwalters512
nwalters512 deleted the ci-yarn-deduplicate branch March 17, 2025 15:54
Sign up for freeto 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.

3 participants

@nwalters512@AbhiPrasad@lforst
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' feat(ci): Run yarn-deduplicate in CI by nwalters512 · Pull Request #15119 · getsentry/sentry-javascript · GitHub
Skip to content

feat(ci): Run yarn-deduplicate in CI - #15119

Merged
lforst merged 16 commits into
getsentry:developfrom
nwalters512:ci-yarn-deduplicate
Mar 17, 2025
Merged

feat(ci): Run yarn-deduplicate in CI#15119
lforst merged 16 commits into
getsentry:developfrom
nwalters512:ci-yarn-deduplicate

Conversation

@nwalters512

Copy link
Copy Markdown
Contributor

This PR introduces a CI step to use yarn-deduplicate to ensure that packages are maximally deduped. This makes version upgrades easier and minimizes the amount of packages that need to be installed by Yarn.

I added this to the lint job, as that seemed the best place for it. Happy to move it elsewhere if desired.

Related to #14968.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@AbhiPrasad 👋 do you have any interest in merging this? I think it'll be a net positive for the project and should make it easier to keep dependencies up-to-date.

@AbhiPrasad

Copy link
Copy Markdown
Contributor

Hey @nwalters512 sorry for the delay! was on vacation and then a bunch of other things came up.

I agree this is a net positive, just need to find some time to set aside to merge it in and update team process stuff for us internally.

@AbhiPrasadAbhiPrasad self-assigned this Mar 4, 2025
@lforstlforst assigned lforst and unassigned AbhiPrasadMar 5, 2025
@lforst
lforst enabled auto-merge (squash) March 10, 2025 11:59
@lforst

Copy link
Copy Markdown
Contributor

E2E tests do not seem to like the deduplication. Will close for now.

@lforstlforst closed this Mar 10, 2025
auto-merge was automatically disabled March 10, 2025 13:40

Pull request was closed

@lforst

Copy link
Copy Markdown
Contributor

It's probably an issue with how we defined deps in our packages but not sure.

@lforst

Copy link
Copy Markdown
Contributor

Nvm, seems to fail on develop too

@lforstlforst reopened this Mar 10, 2025
@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@lforst one potential way to narrow down the issue is to dedupe dependencies one-by-one in separate PRs until we find the one that breaks the E2E tests (currently seems to just be nextjs-13). I'd be happy to open those PRs. Unfortunately, this is difficult for me to test localls, as I can't even get the E2E tests to pass locally on the develop branch.

@lforst

Copy link
Copy Markdown
Contributor

@nwalters512 I'd love to merge this but I can't justify spending time on it. None of this is user facing unfortunately.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@lforst understood, hence my offer to continue driving this forward myself. If you don't even want to spend time reviewing other dependency bump/dedupe PRs, I'd understand that too though.

@lforst

Copy link
Copy Markdown
Contributor

@nwalters512 Oh if you manage to make the tests pass please go ahead 😄 I they pass we can merge. I just won't spend time on it any longer I think.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I'll see what I can do! The fact that just changing dependencies within their semver-compatible versions can make tests fail is a bit of a red flag, so there's probably some interesting deeper issue here.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

First round of this is #15639, all tests pass after deduping all @babel dependencies.

Lms24 pushed a commit that referenced this pull request Mar 13, 2025
Splitting this out of #15119 in the hope that gradually deduping things
will help identify which dependency is causing the Next E2E tests to
fail in that PR.
@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I was able to get the nextjs-13 tests passing locally for me on develop. They start failing with a timeout as soon as rollup is deduplicated:

 1) [chromium] › tests/client/pages-dir-pageload.test.ts:50:5 › should create a pageload transaction with correct name when an error occurs in getServerSideProps Test timeout of 30000ms exceeded.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I've narrowed it down to rollup@4.34.0. Things work with v4.33.0, and fail with v4.34.0, the next version that was published.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I believe Rollup is stripping properties out of an object when it shouldn't be.

Here's the relevant code:

constgetInitialPropsWrappers: Record<string,any>={
'/_app': Sentry.wrapAppGetInitialPropsWithSentry,
'/_document': Sentry.wrapDocumentGetInitialPropsWithSentry,
'/_error': Sentry.wrapErrorGetInitialPropsWithSentry,
};

Here's how it looks when things work (old version of Rollup):

constgetInitialPropsWrappers={'/_app': Sentry.wrapAppGetInitialPropsWithSentry,'/_document': Sentry.wrapDocumentGetInitialPropsWithSentry,'/_error': Sentry.wrapErrorGetInitialPropsWithSentry,};

And when things break (newest version of Rollup):

constgetInitialPropsWrappers={};

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I opened #15651 to independently upgrade to the latest version of Rollup and fix the bug that it caused in the @sentry/nextjs package.

@lforst
lforst merged commit 16e40f4 into getsentry:developMar 17, 2025
@nwalters512
nwalters512 deleted the ci-yarn-deduplicate branch March 17, 2025 15:54
Sign up for freeto 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.

3 participants

@nwalters512@AbhiPrasad@lforst
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' feat(ci): Run yarn-deduplicate in CI by nwalters512 · Pull Request #15119 · getsentry/sentry-javascript · GitHub
Skip to content

feat(ci): Run yarn-deduplicate in CI - #15119

Merged
lforst merged 16 commits into
getsentry:developfrom
nwalters512:ci-yarn-deduplicate
Mar 17, 2025
Merged

feat(ci): Run yarn-deduplicate in CI#15119
lforst merged 16 commits into
getsentry:developfrom
nwalters512:ci-yarn-deduplicate

Conversation

@nwalters512

Copy link
Copy Markdown
Contributor

This PR introduces a CI step to use yarn-deduplicate to ensure that packages are maximally deduped. This makes version upgrades easier and minimizes the amount of packages that need to be installed by Yarn.

I added this to the lint job, as that seemed the best place for it. Happy to move it elsewhere if desired.

Related to #14968.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@AbhiPrasad 👋 do you have any interest in merging this? I think it'll be a net positive for the project and should make it easier to keep dependencies up-to-date.

@AbhiPrasad

Copy link
Copy Markdown
Contributor

Hey @nwalters512 sorry for the delay! was on vacation and then a bunch of other things came up.

I agree this is a net positive, just need to find some time to set aside to merge it in and update team process stuff for us internally.

@AbhiPrasadAbhiPrasad self-assigned this Mar 4, 2025
@lforstlforst assigned lforst and unassigned AbhiPrasadMar 5, 2025
@lforst
lforst enabled auto-merge (squash) March 10, 2025 11:59
@lforst

Copy link
Copy Markdown
Contributor

E2E tests do not seem to like the deduplication. Will close for now.

@lforstlforst closed this Mar 10, 2025
auto-merge was automatically disabled March 10, 2025 13:40

Pull request was closed

@lforst

Copy link
Copy Markdown
Contributor

It's probably an issue with how we defined deps in our packages but not sure.

@lforst

Copy link
Copy Markdown
Contributor

Nvm, seems to fail on develop too

@lforstlforst reopened this Mar 10, 2025
@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@lforst one potential way to narrow down the issue is to dedupe dependencies one-by-one in separate PRs until we find the one that breaks the E2E tests (currently seems to just be nextjs-13). I'd be happy to open those PRs. Unfortunately, this is difficult for me to test localls, as I can't even get the E2E tests to pass locally on the develop branch.

@lforst

Copy link
Copy Markdown
Contributor

@nwalters512 I'd love to merge this but I can't justify spending time on it. None of this is user facing unfortunately.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@lforst understood, hence my offer to continue driving this forward myself. If you don't even want to spend time reviewing other dependency bump/dedupe PRs, I'd understand that too though.

@lforst

Copy link
Copy Markdown
Contributor

@nwalters512 Oh if you manage to make the tests pass please go ahead 😄 I they pass we can merge. I just won't spend time on it any longer I think.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I'll see what I can do! The fact that just changing dependencies within their semver-compatible versions can make tests fail is a bit of a red flag, so there's probably some interesting deeper issue here.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

First round of this is #15639, all tests pass after deduping all @babel dependencies.

Lms24 pushed a commit that referenced this pull request Mar 13, 2025
Splitting this out of #15119 in the hope that gradually deduping things
will help identify which dependency is causing the Next E2E tests to
fail in that PR.
@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I was able to get the nextjs-13 tests passing locally for me on develop. They start failing with a timeout as soon as rollup is deduplicated:

 1) [chromium] › tests/client/pages-dir-pageload.test.ts:50:5 › should create a pageload transaction with correct name when an error occurs in getServerSideProps Test timeout of 30000ms exceeded.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I've narrowed it down to rollup@4.34.0. Things work with v4.33.0, and fail with v4.34.0, the next version that was published.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I believe Rollup is stripping properties out of an object when it shouldn't be.

Here's the relevant code:

constgetInitialPropsWrappers: Record<string,any>={
'/_app': Sentry.wrapAppGetInitialPropsWithSentry,
'/_document': Sentry.wrapDocumentGetInitialPropsWithSentry,
'/_error': Sentry.wrapErrorGetInitialPropsWithSentry,
};

Here's how it looks when things work (old version of Rollup):

constgetInitialPropsWrappers={'/_app': Sentry.wrapAppGetInitialPropsWithSentry,'/_document': Sentry.wrapDocumentGetInitialPropsWithSentry,'/_error': Sentry.wrapErrorGetInitialPropsWithSentry,};

And when things break (newest version of Rollup):

constgetInitialPropsWrappers={};

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I opened #15651 to independently upgrade to the latest version of Rollup and fix the bug that it caused in the @sentry/nextjs package.

@lforst
lforst merged commit 16e40f4 into getsentry:developMar 17, 2025
@nwalters512
nwalters512 deleted the ci-yarn-deduplicate branch March 17, 2025 15:54
Sign up for freeto 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.

3 participants

@nwalters512@AbhiPrasad@lforst
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); feat(ci): Run yarn-deduplicate in CI by nwalters512 · Pull Request #15119 · getsentry/sentry-javascript · GitHub
Skip to content

feat(ci): Run yarn-deduplicate in CI - #15119

Merged
lforst merged 16 commits into
getsentry:developfrom
nwalters512:ci-yarn-deduplicate
Mar 17, 2025
Merged

feat(ci): Run yarn-deduplicate in CI#15119
lforst merged 16 commits into
getsentry:developfrom
nwalters512:ci-yarn-deduplicate

Conversation

@nwalters512

Copy link
Copy Markdown
Contributor

This PR introduces a CI step to use yarn-deduplicate to ensure that packages are maximally deduped. This makes version upgrades easier and minimizes the amount of packages that need to be installed by Yarn.

I added this to the lint job, as that seemed the best place for it. Happy to move it elsewhere if desired.

Related to #14968.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@AbhiPrasad 👋 do you have any interest in merging this? I think it'll be a net positive for the project and should make it easier to keep dependencies up-to-date.

@AbhiPrasad

Copy link
Copy Markdown
Contributor

Hey @nwalters512 sorry for the delay! was on vacation and then a bunch of other things came up.

I agree this is a net positive, just need to find some time to set aside to merge it in and update team process stuff for us internally.

@AbhiPrasadAbhiPrasad self-assigned this Mar 4, 2025
@lforstlforst assigned lforst and unassigned AbhiPrasadMar 5, 2025
@lforst
lforst enabled auto-merge (squash) March 10, 2025 11:59
@lforst

Copy link
Copy Markdown
Contributor

E2E tests do not seem to like the deduplication. Will close for now.

@lforstlforst closed this Mar 10, 2025
auto-merge was automatically disabled March 10, 2025 13:40

Pull request was closed

@lforst

Copy link
Copy Markdown
Contributor

It's probably an issue with how we defined deps in our packages but not sure.

@lforst

Copy link
Copy Markdown
Contributor

Nvm, seems to fail on develop too

@lforstlforst reopened this Mar 10, 2025
@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@lforst one potential way to narrow down the issue is to dedupe dependencies one-by-one in separate PRs until we find the one that breaks the E2E tests (currently seems to just be nextjs-13). I'd be happy to open those PRs. Unfortunately, this is difficult for me to test localls, as I can't even get the E2E tests to pass locally on the develop branch.

@lforst

Copy link
Copy Markdown
Contributor

@nwalters512 I'd love to merge this but I can't justify spending time on it. None of this is user facing unfortunately.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

@lforst understood, hence my offer to continue driving this forward myself. If you don't even want to spend time reviewing other dependency bump/dedupe PRs, I'd understand that too though.

@lforst

Copy link
Copy Markdown
Contributor

@nwalters512 Oh if you manage to make the tests pass please go ahead 😄 I they pass we can merge. I just won't spend time on it any longer I think.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I'll see what I can do! The fact that just changing dependencies within their semver-compatible versions can make tests fail is a bit of a red flag, so there's probably some interesting deeper issue here.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

First round of this is #15639, all tests pass after deduping all @babel dependencies.

Lms24 pushed a commit that referenced this pull request Mar 13, 2025
Splitting this out of #15119 in the hope that gradually deduping things
will help identify which dependency is causing the Next E2E tests to
fail in that PR.
@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I was able to get the nextjs-13 tests passing locally for me on develop. They start failing with a timeout as soon as rollup is deduplicated:

 1) [chromium] › tests/client/pages-dir-pageload.test.ts:50:5 › should create a pageload transaction with correct name when an error occurs in getServerSideProps Test timeout of 30000ms exceeded.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I've narrowed it down to rollup@4.34.0. Things work with v4.33.0, and fail with v4.34.0, the next version that was published.

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I believe Rollup is stripping properties out of an object when it shouldn't be.

Here's the relevant code:

constgetInitialPropsWrappers: Record<string,any>={
'/_app': Sentry.wrapAppGetInitialPropsWithSentry,
'/_document': Sentry.wrapDocumentGetInitialPropsWithSentry,
'/_error': Sentry.wrapErrorGetInitialPropsWithSentry,
};

Here's how it looks when things work (old version of Rollup):

constgetInitialPropsWrappers={'/_app': Sentry.wrapAppGetInitialPropsWithSentry,'/_document': Sentry.wrapDocumentGetInitialPropsWithSentry,'/_error': Sentry.wrapErrorGetInitialPropsWithSentry,};

And when things break (newest version of Rollup):

constgetInitialPropsWrappers={};

@nwalters512

Copy link
Copy Markdown
ContributorAuthor

I opened #15651 to independently upgrade to the latest version of Rollup and fix the bug that it caused in the @sentry/nextjs package.

@lforst
lforst merged commit 16e40f4 into getsentry:developMar 17, 2025
@nwalters512
nwalters512 deleted the ci-yarn-deduplicate branch March 17, 2025 15:54
Sign up for freeto 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.

3 participants

@nwalters512@AbhiPrasad@lforst