-
Notifications
You must be signed in to change notification settings - Fork 254
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: move to src, add TypeScript and rollup #2296
base: dash-13423-2
Are you sure you want to change the base?
Conversation
bin/local-test-util
Outdated
await ex(`npm`, [ `pack`, `--verbose`, `packages/${n}/` ]) | ||
let packageLocation = `packages/${n}/` | ||
if (n === 'plugin-angular') packageLocation += 'dist/' | ||
await ex(`npm`, [ `pack`, `--verbose`, packageLocation ]) |
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.
cherry pick from #2300
@@ -1,6 +1,7 @@ | |||
const testsForPackage = (packageName) => `<rootDir>/packages/${packageName}/**/*.test.[jt]s?(x)` | |||
|
|||
const project = (displayName, packageNames, config = {}) => ({ | |||
resolver: '<rootDir>/jest/node-exports-resolver', |
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.
the jest version being used doesn't support package.exports
so we need to use a custom resolver. This is largely based on https://www.npmjs.com/package/jest-node-exports-resolver but required further changes:
- Using pkg-up to find the right package json for monorepo packages as the existing logic wasn't working
- Setting some default "conditions" as
jest-node-exports-resolver
out of the box no longer supports our version of jest (see Doesn't work for older versions of Jest k-g-a/jest-node-exports-resolver#17)
An attempt to update jest was previously made (#2231) but there were issues because the helpers we use to run electron tests are no longer maintained and do not work with newwer versions of jest.
As alluded to in the previous PR its possible we could work around this by using multiple versions of jest (if a particular package needs a different version temporarily) rather than the single centralized way it is managed now.
@@ -1,8 +1,40 @@ | |||
{ | |||
"name": "@bugsnag/core", | |||
"main": "index.js", | |||
"main": "src/index.js", |
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.
In this PR the package source is moved to a src
folder but the package exports are still referring to the source (not the generated TypeScript). This allows the file move to be separated from the TypeScript conversion allowing for smaller PRs that are easier to review
"version": "8.1.1", | ||
"types": "types/index.d.ts", | ||
"exports": { |
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.
due to the introduction of a src
folder existing import references such as @bugsnag/core/lib/es-utils/map
require an export map for them to point to the correct folder (src for now, will migrate to dist is subsequent PRs)
Presumably we'll want to get back to exporting/importing everything from just @bugsnag/core
in subsequent PRs
"scripts": { | ||
"size": "../../bin/size dist/bugsnag.min.js", | ||
"clean": "rm -fr dist && mkdir dist", | ||
"build": "npm run clean && npm run build:npm", | ||
"build:npm": "rollup --config rollup.config.npm.mjs" | ||
}, |
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.
TypeScript compilation is added (with support for .js files) to the build step but the dist assets are not yet being used
"extends": "../../tsconfig.json", | ||
"compilerOptions": { | ||
"lib": [ "dom", "es2022" ], /* Specify library files to be included in the compilation. */ | ||
"allowJs": 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.
note: allowJs true
"compilerOptions": { | ||
"lib": [ "dom", "es2022" ], /* Specify library files to be included in the compilation. */ | ||
"allowJs": true, | ||
"outDir": "dist", |
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 was having some issues with allowJs and for unclear reasons specifying this solved them. See https://stackoverflow.com/questions/42609768/typescript-error-cannot-write-file-because-it-would-overwrite-input-file
@@ -1,9 +1,11 @@ | |||
{ | |||
"compilerOptions": { | |||
"target": "es5", | |||
"module": "commonjs", | |||
"module": "es2015", | |||
"moduleResolution": "bundler", |
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.
this allows TypeScript to work with package.exports
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.
@@ -6,8 +6,6 @@ import assign from '@bugsnag/core/lib/es-utils/assign' | |||
import { schema as baseConfig } from '@bugsnag/core/config' | |||
import browserConfig from './config' | |||
|
|||
import Event from '@bugsnag/core/event' |
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.
Using this export from '@bugsnag/core/event'
was causing '@bugsnag/core/event'
to appear in the generated types, which was causing issues with some older versions of TypeScript because it would need to resort to package.exports
, which requires a certain version of TypeScript and moduleResolution: bundler
.
Goal
Design
Changeset
Testing