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.

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

core: use dist assets for root export #2305

Merged
merged 6 commits into from
Feb 11, 2025
Merged

Conversation

djskinner
Copy link
Contributor

@djskinner djskinner commented Feb 4, 2025

For the default export of @bugsnag/core switch to using the dist assets created by rollup/TypeScript. This means all imports of @bugsnag/core will now use the compiled assets rather than the source code. Note that imports such as @bugsnag/core/breadcrumb are still pointing directly at the source code for now

Copy link

github-actions bot commented Feb 4, 2025

@bugsnag/browser bundle size diff

Minified Minfied + Gzipped
Before 50.02 kB 15.41 kB
After 50.02 kB 15.41 kB
± No change No change

code coverage diff

<temporarily disabled>

Generated by 🚫 dangerJS against 4233e12

Base automatically changed from plat-12947-3 to plat-12947 February 11, 2025 08:22
Base automatically changed from plat-12947 to integration/typescript February 11, 2025 09:45
@@ -54,7 +54,7 @@ function getPackageJson (packageName) {
return require(`${packageName}/package.json`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In summary I had to change the temporary jest resolver to fix some issues with resolving packageName=@bugsnag/core, submoduleName=. by removing the && submoduleName !== '.' condition. But then needed to return null instead of erroring for node modules that resolve with e.g. packageName=https, submoduleName=.

Hopefully we'll be able to bump jest later and we'll be able to drop this

@@ -1,22 +1,23 @@
import Bugsnag, { Client, Config } from '..'
Copy link
Contributor Author

@djskinner djskinner Feb 11, 2025

Choose a reason for hiding this comment

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

@bugsnag/core never had a default export but this is a confusion from mising cjs exports and esm imports. In cjs it would have been:

const { Client, Breadcrumb, Event, Session } = require('@bugsnag/core')` which makes sense against the deleted index file above

@@ -6,15 +6,98 @@ import {
OnSessionCallback,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the question of public versus internal interfaces for the notifiers themselves we can address that later. However I don't think it makes much sense at all for @bugsnag/core to have this concept. We don't document/expect our customers to use @bugsnag/core directly anyway so we can just keep things simple and export the actual interfaces.

For now though, we're still using the manual types - we can't switch to generated types until all the JS files that make up the external interfaces are converted to TypeScript otherwise invalid types are generated.

Comment on lines -9 to +10
"import": "./src/index.js",
"default": "./src/index.js"
"import": "./dist/index.mjs",
"default": "./dist/index.cjs"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the main change, the rest of this PR are the consequences

@djskinner djskinner marked this pull request as ready for review February 11, 2025 11:23
@djskinner djskinner requested a review from gingerbenw February 11, 2025 11:41
@djskinner djskinner merged commit 7c5083b into integration/typescript Feb 11, 2025
55 checks passed
@djskinner djskinner deleted the plat-12947-4 branch February 11, 2025 12:23
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.

2 participants