Typescript Core/Browser implementation by HazAT · Pull Request #1149 · getsentry/sentry-javascript

HazAT · 2017-11-25T20:13:23Z

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.ts for the implementation of the public API.
Also core/__tests__/client.ts shows a way of how to use it.

Structure

There is our core package called @sentry/core.
@sentry/browser is one adapter to use with the core.
Currently it's just wrapping raven-js until we decide to rewrite it.
Also, our react-native or the new Cordova SDK should build on this architecture.

Publish @sentry/core @sentry/browser
Fix @sentry/browser package.json dependency
Add .npmignore to exclude packages from raven-js
Readme how to run the packages
Delete https://github.com/getsentry/sentry-js-proposal repo
Delete fork

jan-auer

Seriously, awesome work Daniel! 🥇

Due to the size of this PR, I have a higher number of comments as well. Most of this is opinionated and or just minor stuff, but I'd like to discuss the bigger ones before this gets merged.

jan-auer · 2017-11-29T13:28:09Z

.vscode/settings.json

@@ -0,0 +1,18 @@
+{
+    "[typescript]": {


I'd also add editor.rulers and editor.tabSize here.

jan-auer · 2017-11-29T15:47:51Z

packages/browser/package.json

+    "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"


This doesn't really make sense as you have to re-run dist on changes, no?

jan-auer · 2017-11-29T15:49:54Z

packages/browser/package.json

+  "engines": {
+    "node": ">=6.9.5 <9.0.0",
+    "npm": ">=3.10.7 <6.0.0",
+    "yarn": ">=1.0.2 <2.0.0"


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?

jan-auer · 2017-11-29T15:52:14Z

packages/browser/__tests__/index.ts

+
+describe('Browser Interface', () => {
+  test('sending a message', async done => {
+    let countExcpects = 0;


Typo: countExpects

jan-auer · 2017-11-29T15:57:35Z

packages/browser/lib/Browser.ts

+
+  constructor(client: Client, public options: IBrowserOptions = {}) {
+    this.client = client;
+    return this;


No need to return this here.

jan-auer · 2017-11-29T16:46:16Z

packages/core/lib/Interfaces/Context.ts

+  context[key] = value;
+}
+
+export function merge<T extends IContext, K extends keyof T>(


Just a suggestion: I'd call it mergeIn, as merge usually refers to shallow-merging all keys of two or more objects.

jan-auer · 2017-11-29T16:47:28Z

packages/core/lib/Interfaces/Context.ts

+    set(context, key, {});
+  }
+  if (value === undefined) {
+    delete context[key];


Are you sure you want this behavior? Usually merge or assign functions as well as the object spread operator ignore sources that are falsy.

So I took this merge function basically from raven-js and I guess we want to keep the same logic.

jan-auer · 2017-11-29T16:48:56Z

packages/core/lib/Sentry.ts

+}
+
+export class SentryError implements Error {
+  public name = 'SentryError';


Isn't this implicitly defined if you extend Error?

jan-auer · 2017-11-29T16:54:48Z

packages/core/package.json

+    "test": "npm run dist && jest",
+    "test:watch": "jest --watch --notify"
+  },
+  "jest": {


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" }

jan-auer · 2017-11-29T16:56:45Z

packages/browser/tsconfig.json

+        ],
+        "lib": ["es2015", "dom"],
+        "declaration": false,
+        "allowJs": true,


@sentry/core uses false here. Purpose?

HazAT · 2017-11-29T17:11:04Z

HazAT · 2017-12-05T16:05:55Z

Note:
We cannot use yarn workspaces yet since it requires the root package.json to be private:true.

yarn install v1.3.2
info No lockfile found.
error Workspaces can only be enabled in private projects
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

HazAT · 2017-12-06T10:28:32Z

I don't know why but travis has a hard time with this PR
It works without problems in my fork: https://travis-ci.org/HazAT/raven-js

jan-auer · 2017-12-06T14:00:05Z

🚀 it

Update .eslintignore

jan-auer · 2017-12-07T14:05:36Z

🎉 🎆

HazAT added 30 commits October 10, 2017 14:37

feat: Initial commit

39b726b

feat: Add tests with jest

bd0281e

feat: Add basic tests

1cb810a

feat: Strip tests

fb4ffad

feat: Add tests for captureEvent/message

6f42294

feat: Steamline options

5a0660a

feat: Added sdk options

58d9af8

feat: Write readme

0d87d90

feat: Move to new folder structure

6b366fb

fix: package.json

66baabf

feat: Add js tests

74bebed

feat: Add webpack in platform-browser to get bundle

01b2a44

fix: rename platform folder

554b67e

fix: Remove Set

068574e

meta: Add todos

9d104de

feat: Add headless chrome tests

19d34c0

ref: Rename Sdk to Integration

9c5b1c1

ref: Integration renaming

845e036

ref: Rename to adapter

b153149

fix: Implement real captureException and captureMessage

48ce5de

meta: Change naming

00f17c9

ref: Rename main files

14ec5cd

feat: Add sharedClient sentry

4ff9502

feat: Add dsn parsing with tests

783d75f

feat: Add tests for wrong dsn

86062ef

Merge remote-tracking branch 'remotes/foo/merge' into feature/packages

ac899be

fix: Naming of SentryError

41e7726

feat: Add setOptions function to adapters

51997df

feat: Add global create function

cb6e611

feat: Remove multiple adapter architecture

5ae9476

HazAT requested review from jan-auer and mitsuhiko November 29, 2017 13:20

jan-auer requested changes Nov 29, 2017

View reviewed changes

HazAT added 7 commits November 30, 2017 14:58

fix: Different code review changes

76c17e7

ref: Code review changes

cfa14b3

ref: make install return a promise

6044d17

fix: Browser tests

663140b

fix: Remove node ts reference

3bfdb0b

meta: Add documentation

219bf92

feat: Add npmignore

43e8ccf

HazAT added 5 commits December 5, 2017 17:08

feat: package.json version changes

b001ffb

fix(ci): travis tests

428e022

ci: Fix travis build

93dc09f

ci: Fix travis and skip install

14cb9d5

ci: Increase timeout to 60sec

3fb0d15

ci: Remove increased timeout

fcab234

jan-auer approved these changes Dec 6, 2017

View reviewed changes

HazAT added 5 commits December 6, 2017 17:27

feat: Use rollup instead of webpack

3895cfe

feat: Rename Browser to SentryBrowser

7aa57db

feat: Add size:check npm script

a654394

Update .eslintignore

fix: package.json main

8773125

feat: Export ISentryBrowserOptions

dc5f3e5

HazAT merged commit 5d2685f into getsentry:master Dec 7, 2017

RuslanZavacky mentioned this pull request Dec 18, 2017

Add fetchParameters to configure fetch #1177

Merged

3 tasks

kamilogorek mentioned this pull request Jan 18, 2018

ignoreErrors not working getsentry/sentry#6100

Closed

Uh oh!

Conversation

HazAT commented Nov 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposal

Structure

Uh oh!

jan-auer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HazAT commented Nov 29, 2017

Uh oh!

HazAT commented Dec 5, 2017

Uh oh!

HazAT commented Dec 6, 2017

Uh oh!

jan-auer commented Dec 6, 2017

Uh oh!

jan-auer commented Dec 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

HazAT commented Nov 25, 2017 •

edited

Loading