Typescript Core/Browser implementation #1149
Conversation
.vscode/settings.json
Outdated
| @@ -0,0 +1,18 @@ | |||
| { | |||
| "[typescript]": { | |||
There was a problem hiding this comment.
I'd also add editor.rulers and editor.tabSize here.
packages/browser/package.json
Outdated
| "clean": "rm -rf ./dist", | ||
| "dist": "npm run clean && tsc -p tsconfig.json && npm run build-browser", | ||
| "test": "npm run dist && jest --forceExit", | ||
| "test:watch": "npm run dist && jest --watch --notify" |
There was a problem hiding this comment.
This doesn't really make sense as you have to re-run dist on changes, no?
packages/browser/package.json
Outdated
| "engines": { | ||
| "node": ">=6.9.5 <9.0.0", | ||
| "npm": ">=3.10.7 <6.0.0", | ||
| "yarn": ">=1.0.2 <2.0.0" |
There was a problem hiding this comment.
Pinning npm and yarn this restrictively might cause pain once they release a new major version and there are no incompatibilities. Same goes for node - v9 and v10 are already in development and should actually work perfectly fine... Is there a specific reason you specify an upper bound?
packages/browser/__tests__/index.ts
Outdated
|
|
||
| describe('Browser Interface', () => { | ||
| test('sending a message', async done => { | ||
| let countExcpects = 0; |
packages/browser/lib/Browser.ts
Outdated
|
|
||
| constructor(client: Client, public options: IBrowserOptions = {}) { | ||
| this.client = client; | ||
| return this; |
| context[key] = value; | ||
| } | ||
|
|
||
| export function merge<T extends IContext, K extends keyof T>( |
There was a problem hiding this comment.
Just a suggestion: I'd call it mergeIn, as merge usually refers to shallow-merging all keys of two or more objects.
| set(context, key, {}); | ||
| } | ||
| if (value === undefined) { | ||
| delete context[key]; |
There was a problem hiding this comment.
Are you sure you want this behavior? Usually merge or assign functions as well as the object spread operator ignore sources that are falsy.
There was a problem hiding this comment.
So I took this merge function basically from raven-js and I guess we want to keep the same logic.
| } | ||
|
|
||
| export class SentryError implements Error { | ||
| public name = 'SentryError'; |
There was a problem hiding this comment.
Isn't this implicitly defined if you extend Error?
packages/core/package.json
Outdated
| "test": "npm run dist && jest", | ||
| "test:watch": "jest --watch --notify" | ||
| }, | ||
| "jest": { |
There was a problem hiding this comment.
By default, jest applies the "jsdom" environment which essentially polyfills some browser APIs. However, this means that even though your tests pass, the code might not run in an actual node process. However, since you want to run it in both the browser and node, it might make sense to run tests in both environments and use the --env option to switch between them. If you don't want to do this, declare "testEnvironment": "node" here as this is more of a common demeanor.
Example for running both (needs npm-run-all as devDependency):
{
"dist": "npm run clean && tsc -p tsconfig.json",
"test:browser": "jest --env=jsdom",
"test:node": "jest --env=node",
"test": "npm-run-all dist test:browser test:node"
}
packages/browser/tsconfig.json
Outdated
| ], | ||
| "lib": ["es2015", "dom"], | ||
| "declaration": false, | ||
| "allowJs": true, |
There was a problem hiding this comment.
@sentry/core uses false here. Purpose?
|
|
|
Note: |
|
I don't know why but travis has a hard time with this PR |
|
🚀 it |
|
🎉 🎆 |
Proposal
This should become our Core for all our javascript related SDKs.
It's written in typescript to provide type safety BUT the javascript
API should also be easily human readable/usable.
You can take a look at
core/lib/Client.tsfor the implementation of the public API.Also
core/__tests__/client.tsshows a way of how to use it.Structure
There is our core package called
@sentry/core.@sentry/browseris one adapter to use with the core.Currently it's just wrapping
raven-jsuntil we decide to rewrite it.Also, our
react-nativeor the newCordovaSDK should build on this architecture.@sentry/core@sentry/browser@sentry/browserpackage.json dependency.npmignoreto exclude packages fromraven-js