feat(typescript): Add TypeScript compatibility - #13786

Merged
lynnagara merged 44 commits into
masterfrom
typescript
Jul 22, 2019
Merged

feat(typescript): Add TypeScript compatibility#13786
lynnagara merged 44 commits into
masterfrom
typescript

Conversation

@HazAT

@HazATHazAT commented Jun 24, 2019•
edited by lynnagara
Loading

Copy link
Copy Markdown
Member

This PR is intended to serve as a demonstration for how we could write TypeScript code in Sentry. It migrates the following code to TypeScript:

  • all Discover functionality
  • some utility functions

It depends on:

@dashed

dashed commented Jun 24, 2019•
edited
Loading

Copy link
Copy Markdown
Member

At the time of this message, Typescript doesn't support optional chaining proposal (https://github.com/tc39/proposal-optional-chaining), which is stage 2 microsoft/TypeScript#16 .

Some suggestions:

@dashed

dashed commented Jun 24, 2019

Copy link
Copy Markdown
Member

tslint is going to be deprecated:

@lynnagara
lynnagaraforce-pushed the typescript branch 2 times, most recently from e38d0db to 289b539CompareJune 24, 2019 20:39
lynnagara
lynnagara reviewed Jun 24, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/aggregations/index.tsx Outdated
lynnagara added a commit that referenced this pull request Jun 24, 2019
@lynnagara
ref: Convert app.jsx to ES6
9ba73d0
Uses withConfig HoC instead of connecting to the ConfigStore directly
This is required for #13786
@lynnagaralynnagara mentioned this pull request Jun 24, 2019
Merged
lynnagara added a commit that referenced this pull request Jun 24, 2019
@lynnagara
ref: Remove optional chaining
4c9eba2
This is required for #13786
@lynnagaralynnagara mentioned this pull request Jun 25, 2019
Merged
dashed
dashed reviewed Jun 25, 2019
View reviewed changes
Comment thread.eslintrc.js
'@typescript-eslint/no-unused-vars': 'off',
},
},
],

@dasheddashedJun 25, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@typescript-eslint/no-unused-vars rule was causing issues for imported types. see: typescript-eslint/typescript-eslint#363

I've added this as per https://43081j.com/2019/02/using-eslint-with-typescript

dashed
dashed reviewed Jun 25, 2019
View reviewed changes
Comment thread.eslintrc.js Outdated

@dasheddashedJun 25, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Caveat: jsx-eslint/eslint-plugin-react#2066

@dasheddashed added the WIP label Jun 25, 2019
lynnagara added a commit that referenced this pull request Jun 25, 2019
@lynnagara
ref: Convert app.jsx to ES6 (#13798)
634c2a7
Uses withConfig HoC instead of connecting to the ConfigStore directly
This is required for #13786
lynnagara added a commit that referenced this pull request Jun 25, 2019
@lynnagara
ref: Remove optional chaining
f423a42
This is required for #13786
pullBot pushed a commit to Mattlk13/sentry-1 that referenced this pull request Jun 25, 2019
@lynnagara
ref: Remove optional chaining (getsentry#13800)
ff3c330
This is required for getsentry#13786
@lynnagara
lynnagaraforce-pushed the typescript branch 2 times, most recently from 66506af to aac7d03CompareJune 25, 2019 20:01
@lynnagaralynnagara mentioned this pull request Jun 25, 2019
Merged
lynnagara
lynnagara reviewed Jun 27, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/queryBuilder.tsx
const type = SPECIAL_TAGS[tags.tags_key] || 'string';
.filter((tag: TagData) => !HIDDEN_TAGS.includes(tag.tags_key))
.map((tag: TagData) => {
const type = SPECIAL_TAGS[tag.tags_key] || 'string';

@lynnagaralynnagaraJun 27, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This was a good bug, TypeScript made it immediately obvious

lynnagara
lynnagara reviewed Jun 27, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/discover.tsx
@@ -409,7 +429,6 @@ export default class OrganizationDiscover extends React.Component {
isFetchingQuery={isFetchingQuery}
onUpdateField={this.updateField}
onRunQuery={this.runQuery}
onReset={this.reset}

@lynnagaralynnagaraJun 27, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 for errors on unused props

@lynnagaralynnagara removed the WIP label Jun 28, 2019
@lynnagaralynnagara changed the title [WIP] feat: Add typescript compatibilityfeat(typescript): Add TypeScript compatibilityJun 28, 2019
lynnagara
lynnagara reviewed Jul 1, 2019
View reviewed changes
Comment threadpackage.json
"@sentry/typescript": "^5.3.0",
"@types/lodash": "^4.14.134",
"@types/react-dom": "^16.8.4",
"@types/moment-timezone": "^0.5.12",

@lynnagaralynnagaraJul 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@HazAT Do you know if/why these need to be dependencies? Would be really nice to move these to devDependencies?

@HazATHazATJul 10, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Sorry for the late reply. I thought we need this since we do different stuff if you call
make develop or yarn install.
make develop apparently on installs dependencies whereby yarn install installs all.

So if you do a prod build it would fail because of missing types.

@lynnagaralynnagaraJul 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Gotcha, thanks for the explanation

dashed
dashed approved these changes Jul 10, 2019
View reviewed changes

@dasheddashed left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I took the liberty to rebase this branch against master, and looked through the PR.

Looks good to me 👍

dashed
dashed reviewed Jul 19, 2019
View reviewed changes
Comment threadtsconfig.json
"declarationMap": false,
"allowJs": true,
"allowJs": false,
"noImplicitAny": false,

@dasheddashedJul 19, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@HazAT@lynnagara There are issues with importing React components from *.jsx files, and I don't think redeclaring them as const FooBarAny: any = FooBar; is the right approach to migrate Sentry to TypeScript in the long-term.

Since this file is extending @sentry/typescript/tsconfig.json (https://github.com/getsentry/sentry-javascript/blob/5d670a7781b6e2144809c002e9332e2cc9dd239c/packages/typescript/tsconfig.json), I'm proposing to provide the following overrides that are specific for this project:

 "allowJs": false,
"noImplicitAny": false,

The behavioural changes are as follows:

  • Setting allowJs to be false prevents the TypeScript engine from processing *.jsx files and as well as inferring types of imported js code. This resolves cryptic proptype type issues for React components sourced in *.jsx files .

  • The implications of disabling allowJs means that imported modules from *.jsx files are implied to have an any type. Hence, we need noImplicitAny to be false.

@lynnagara

lynnagara commented Jul 19, 2019

Copy link
Copy Markdown
Member

Deployed to staging, everything looks fine

@lynnagara
lynnagara merged commit 7823ddb into masterJul 22, 2019
@lynnagara
lynnagara deleted the typescript branch July 22, 2019 19:19
HazAT added a commit that referenced this pull request Jul 23, 2019
@HazAT
Merge branch 'master' into feat/apm-v2
8d62f17
* master: (25 commits)
ref(onboarding): Fix install promprt URL (#14106)
fix(app-platform): Allow GET requests for published apps (#14109)
feat: Update Group.get_latest_event to use Snuba event (#14039)
ref(onboarding): Rename wizardNew -> onboarding (#14104)
feat(apm): Update props to address proptype warnings for new transaction attributes (SEN-800) (#14040)
ref(ui): Move and codesplit `ProjectPlugins` (#13952)
feat(typescript): Add TypeScript compatibility (#13786)
ref(templates): Remove unused content block default (#14090)
ref(less): Remove unused admin.less (#14097)
ref(onobarding): Remove old onboarding experience (#14066)
fix(ui) Fix missing conditions in tag bars (#14063)
ref(suspect-commits): Add hook (#14057)
ref(frontend): Segment frontend web urls (#14096)
feat(suspect-commits): Add analytics events (#14080)
feat(servicehooks): Update servicehook URLs (#14093)
license: Remove license headers (#14095)
ref(templates): Remove unused account_nav (#14091)
fix: Disable transaction events in store (#14088)
fix(InstallWizard): Fix exception when InstallWizard completed (#14092)
ref(admin): Fix thrashing on stat charts (#14094)
...
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 20, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dasheddasheddashed approved these changes
@lynnagaralynnagaralynnagara approved these changes

Assignees

@HazATHazAT

@lynnagaralynnagara

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@HazAT@dashed@lynnagara
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n 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;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
feat(typescript): Add TypeScript compatibility - #13786
Merged
lynnagara merged 44 commits into
masterfrom 
typescript
Jul 22, 2019
Merged
feat(typescript): Add TypeScript compatibility#13786
lynnagara merged 44 commits into
masterfrom 
typescript
Conversation
@HazAT
@HazATHazAT
commented
Jun 24, 2019•

edited by lynnagara

Loading

Copy link


Copy Markdown

Member
This PR is intended to serve as a demonstration for how we could write TypeScript code in Sentry. It migrates the following code to TypeScript:
  • all Discover functionality
  • some utility functions
It depends on:
@dashed
dashed
commented
Jun 24, 2019•

edited

Loading

Copy link


Copy Markdown

Member
At the time of this message, Typescript doesn't support optional chaining proposal (https://github.com/tc39/proposal-optional-chaining), which is stage 2 microsoft/TypeScript#16 .
Some suggestions:
@dashed
dashed
commented
Jun 24, 2019

Copy link


Copy Markdown

Member
tslint is going to be deprecated:
@lynnagara
lynnagaraforce-pushed
the
typescript
branch
2 times, most recently
from
e38d0db to
289b539CompareJune 24, 2019 20:39
lynnagara
lynnagara
reviewed
Jun 24, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/aggregations/index.tsx
Outdated

lynnagara added a commit
that referenced
this pull request
Jun 24, 2019
@lynnagara
ref: Convert app.jsx to ES6
9ba73d0
Uses withConfig HoC instead of connecting to the ConfigStore directly
This is required for #13786
@lynnagaralynnagara
mentioned this pull request
Jun 24, 2019
 Merged

lynnagara added a commit
that referenced
this pull request
Jun 24, 2019
@lynnagara
ref: Remove optional chaining
4c9eba2
This is required for #13786
@lynnagaralynnagara
mentioned this pull request
Jun 25, 2019
 Merged

dashed
dashed
reviewed
Jun 25, 2019
View reviewed changes
Comment thread.eslintrc.js
 '@typescript-eslint/no-unused-vars': 'off',
 },
 },
 ],
@dasheddashedJun 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@typescript-eslint/no-unused-vars rule was causing issues for imported types. see: typescript-eslint/typescript-eslint#363
I've added this as per https://43081j.com/2019/02/using-eslint-with-typescript
dashed
dashed
reviewed
Jun 25, 2019
View reviewed changes
Comment thread.eslintrc.js
Outdated

@dasheddashedJun 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Caveat: jsx-eslint/eslint-plugin-react#2066
@dasheddashed
added
the 
WIP

label
Jun 25, 2019
lynnagara added a commit
that referenced
this pull request
Jun 25, 2019
@lynnagara
ref: Convert app.jsx to ES6 (#13798)
634c2a7
Uses withConfig HoC instead of connecting to the ConfigStore directly
This is required for #13786
lynnagara added a commit
that referenced
this pull request
Jun 25, 2019
@lynnagara
ref: Remove optional chaining
f423a42
This is required for #13786
pullBot
pushed a commit
to Mattlk13/sentry-1
that referenced
this pull request
Jun 25, 2019
@lynnagara
ref: Remove optional chaining (getsentry#13800)
ff3c330
This is required for getsentry#13786
@lynnagara
lynnagaraforce-pushed
the
typescript
branch
2 times, most recently
from
66506af to
aac7d03CompareJune 25, 2019 20:01
@lynnagaralynnagara
mentioned this pull request
Jun 25, 2019
 Merged

lynnagara
lynnagara
reviewed
Jun 27, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/queryBuilder.tsx
 const type = SPECIAL_TAGS[tags.tags_key] || 'string';
 .filter((tag: TagData) => !HIDDEN_TAGS.includes(tag.tags_key))
 .map((tag: TagData) => {
 const type = SPECIAL_TAGS[tag.tags_key] || 'string';
@lynnagaralynnagaraJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This was a good bug, TypeScript made it immediately obvious
lynnagara
lynnagara
reviewed
Jun 27, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/discover.tsx
@@ -409,7 +429,6 @@ export default class OrganizationDiscover extends React.Component {
 isFetchingQuery={isFetchingQuery}
 onUpdateField={this.updateField}
 onRunQuery={this.runQuery}
 onReset={this.reset}
@lynnagaralynnagaraJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

👍 for errors on unused props
@lynnagaralynnagara
removed
the 
WIP

label
Jun 28, 2019
@lynnagaralynnagara
changed the title
[WIP] feat: Add typescript compatibilityfeat(typescript): Add TypeScript compatibilityJun 28, 2019
lynnagara
lynnagara
reviewed
Jul 1, 2019
View reviewed changes
Comment threadpackage.json
 "@sentry/typescript": "^5.3.0",
 "@types/lodash": "^4.14.134",
 "@types/react-dom": "^16.8.4",
 "@types/moment-timezone": "^0.5.12",
@lynnagaralynnagaraJul 1, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@HazAT Do you know if/why these need to be dependencies? Would be really nice to move these to devDependencies?
@HazATHazATJul 10, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sorry for the late reply. I thought we need this since we do different stuff if you call
make develop or yarn install.
make develop apparently on installs dependencies whereby yarn install installs all.
So if you do a prod build it would fail because of missing types.
@lynnagaralynnagaraJul 10, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Gotcha, thanks for the explanation
dashed
dashed
approved these changes
Jul 10, 2019
View reviewed changes
@dasheddashed
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I took the liberty to rebase this branch against master, and looked through the PR.
Looks good to me 👍
dashed
dashed
reviewed
Jul 19, 2019
View reviewed changes
Comment threadtsconfig.json
 "declarationMap": false,
 "allowJs": true,
 "allowJs": false,
 "noImplicitAny": false,
@dasheddashedJul 19, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@HazAT@lynnagara There are issues with importing React components from *.jsx files, and I don't think redeclaring them as const FooBarAny: any = FooBar; is the right approach to migrate Sentry to TypeScript in the long-term.
Since this file is extending @sentry/typescript/tsconfig.json (https://github.com/getsentry/sentry-javascript/blob/5d670a7781b6e2144809c002e9332e2cc9dd239c/packages/typescript/tsconfig.json), I'm proposing to provide the following overrides that are specific for this project:
 "allowJs": false,
"noImplicitAny": false,

The behavioural changes are as follows:
  • Setting allowJs to be false prevents the TypeScript engine from processing *.jsx files and as well as inferring types of imported js code. This resolves cryptic proptype type issues for React components sourced in *.jsx files .
  • The implications of disabling allowJs means that imported modules from *.jsx files are implied to have an any type. Hence, we need noImplicitAny to be false.
@lynnagara
lynnagara
commented
Jul 19, 2019

Copy link


Copy Markdown

Member
Deployed to staging, everything looks fine
@lynnagara
lynnagara
merged commit 7823ddb
into
masterJul 22, 2019
@lynnagara
lynnagara
deleted the

typescript

branch
July 22, 2019 19:19
HazAT added a commit
that referenced
this pull request
Jul 23, 2019
@HazAT
Merge branch 'master' into feat/apm-v2
8d62f17
* master: (25 commits)
ref(onboarding): Fix install promprt URL (#14106)
fix(app-platform): Allow GET requests for published apps (#14109)
feat: Update Group.get_latest_event to use Snuba event (#14039)
ref(onboarding): Rename wizardNew -> onboarding (#14104)
feat(apm): Update props to address proptype warnings for new transaction attributes (SEN-800) (#14040)
ref(ui): Move and codesplit `ProjectPlugins` (#13952)
feat(typescript): Add TypeScript compatibility (#13786)
ref(templates): Remove unused content block default (#14090)
ref(less): Remove unused admin.less (#14097)
ref(onobarding): Remove old onboarding experience (#14066)
fix(ui) Fix missing conditions in tag bars (#14063)
ref(suspect-commits): Add hook (#14057)
ref(frontend): Segment frontend web urls (#14096)
feat(suspect-commits): Add analytics events (#14080)
feat(servicehooks): Update servicehook URLs (#14093)
license: Remove license headers (#14095)
ref(templates): Remove unused account_nav (#14091)
fix: Disable transaction events in store (#14088)
fix(InstallWizard): Fix exception when InstallWizard completed (#14092)
ref(admin): Fix thrashing on stat charts (#14094)
...
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 20, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@dasheddasheddashed approved these changes
@lynnagaralynnagaralynnagara approved these changes

Assignees

@HazATHazAT
@lynnagaralynnagara

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@HazAT@dashed@lynnagara

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(typescript): Add TypeScript compatibility - #13786
Merged
lynnagara merged 44 commits into
masterfrom 
typescript
Jul 22, 2019
Merged
feat(typescript): Add TypeScript compatibility#13786
lynnagara merged 44 commits into
masterfrom 
typescript
Conversation
@HazAT
@HazATHazAT
commented
Jun 24, 2019•

edited by lynnagara

Loading

Copy link


Copy Markdown

Member
This PR is intended to serve as a demonstration for how we could write TypeScript code in Sentry. It migrates the following code to TypeScript:
  • all Discover functionality
  • some utility functions
It depends on:
@dashed
dashed
commented
Jun 24, 2019•

edited

Loading

Copy link


Copy Markdown

Member
At the time of this message, Typescript doesn't support optional chaining proposal (https://github.com/tc39/proposal-optional-chaining), which is stage 2 microsoft/TypeScript#16 .
Some suggestions:
@dashed
dashed
commented
Jun 24, 2019

Copy link


Copy Markdown

Member
tslint is going to be deprecated:
@lynnagara
lynnagaraforce-pushed
the
typescript
branch
2 times, most recently
from
e38d0db to
289b539CompareJune 24, 2019 20:39
lynnagara
lynnagara
reviewed
Jun 24, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/aggregations/index.tsx
Outdated

lynnagara added a commit
that referenced
this pull request
Jun 24, 2019
@lynnagara
ref: Convert app.jsx to ES6
9ba73d0
Uses withConfig HoC instead of connecting to the ConfigStore directly
This is required for #13786
@lynnagaralynnagara
mentioned this pull request
Jun 24, 2019
 Merged

lynnagara added a commit
that referenced
this pull request
Jun 24, 2019
@lynnagara
ref: Remove optional chaining
4c9eba2
This is required for #13786
@lynnagaralynnagara
mentioned this pull request
Jun 25, 2019
 Merged

dashed
dashed
reviewed
Jun 25, 2019
View reviewed changes
Comment thread.eslintrc.js
 '@typescript-eslint/no-unused-vars': 'off',
 },
 },
 ],
@dasheddashedJun 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@typescript-eslint/no-unused-vars rule was causing issues for imported types. see: typescript-eslint/typescript-eslint#363
I've added this as per https://43081j.com/2019/02/using-eslint-with-typescript
dashed
dashed
reviewed
Jun 25, 2019
View reviewed changes
Comment thread.eslintrc.js
Outdated

@dasheddashedJun 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Caveat: jsx-eslint/eslint-plugin-react#2066
@dasheddashed
added
the 
WIP

label
Jun 25, 2019
lynnagara added a commit
that referenced
this pull request
Jun 25, 2019
@lynnagara
ref: Convert app.jsx to ES6 (#13798)
634c2a7
Uses withConfig HoC instead of connecting to the ConfigStore directly
This is required for #13786
lynnagara added a commit
that referenced
this pull request
Jun 25, 2019
@lynnagara
ref: Remove optional chaining
f423a42
This is required for #13786
pullBot
pushed a commit
to Mattlk13/sentry-1
that referenced
this pull request
Jun 25, 2019
@lynnagara
ref: Remove optional chaining (getsentry#13800)
ff3c330
This is required for getsentry#13786
@lynnagara
lynnagaraforce-pushed
the
typescript
branch
2 times, most recently
from
66506af to
aac7d03CompareJune 25, 2019 20:01
@lynnagaralynnagara
mentioned this pull request
Jun 25, 2019
 Merged

lynnagara
lynnagara
reviewed
Jun 27, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/queryBuilder.tsx
 const type = SPECIAL_TAGS[tags.tags_key] || 'string';
 .filter((tag: TagData) => !HIDDEN_TAGS.includes(tag.tags_key))
 .map((tag: TagData) => {
 const type = SPECIAL_TAGS[tag.tags_key] || 'string';
@lynnagaralynnagaraJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This was a good bug, TypeScript made it immediately obvious
lynnagara
lynnagara
reviewed
Jun 27, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/discover.tsx
@@ -409,7 +429,6 @@ export default class OrganizationDiscover extends React.Component {
 isFetchingQuery={isFetchingQuery}
 onUpdateField={this.updateField}
 onRunQuery={this.runQuery}
 onReset={this.reset}
@lynnagaralynnagaraJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

👍 for errors on unused props
@lynnagaralynnagara
removed
the 
WIP

label
Jun 28, 2019
@lynnagaralynnagara
changed the title
[WIP] feat: Add typescript compatibilityfeat(typescript): Add TypeScript compatibilityJun 28, 2019
lynnagara
lynnagara
reviewed
Jul 1, 2019
View reviewed changes
Comment threadpackage.json
 "@sentry/typescript": "^5.3.0",
 "@types/lodash": "^4.14.134",
 "@types/react-dom": "^16.8.4",
 "@types/moment-timezone": "^0.5.12",
@lynnagaralynnagaraJul 1, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@HazAT Do you know if/why these need to be dependencies? Would be really nice to move these to devDependencies?
@HazATHazATJul 10, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sorry for the late reply. I thought we need this since we do different stuff if you call
make develop or yarn install.
make develop apparently on installs dependencies whereby yarn install installs all.
So if you do a prod build it would fail because of missing types.
@lynnagaralynnagaraJul 10, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Gotcha, thanks for the explanation
dashed
dashed
approved these changes
Jul 10, 2019
View reviewed changes
@dasheddashed
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I took the liberty to rebase this branch against master, and looked through the PR.
Looks good to me 👍
dashed
dashed
reviewed
Jul 19, 2019
View reviewed changes
Comment threadtsconfig.json
 "declarationMap": false,
 "allowJs": true,
 "allowJs": false,
 "noImplicitAny": false,
@dasheddashedJul 19, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@HazAT@lynnagara There are issues with importing React components from *.jsx files, and I don't think redeclaring them as const FooBarAny: any = FooBar; is the right approach to migrate Sentry to TypeScript in the long-term.
Since this file is extending @sentry/typescript/tsconfig.json (https://github.com/getsentry/sentry-javascript/blob/5d670a7781b6e2144809c002e9332e2cc9dd239c/packages/typescript/tsconfig.json), I'm proposing to provide the following overrides that are specific for this project:
 "allowJs": false,
"noImplicitAny": false,

The behavioural changes are as follows:
  • Setting allowJs to be false prevents the TypeScript engine from processing *.jsx files and as well as inferring types of imported js code. This resolves cryptic proptype type issues for React components sourced in *.jsx files .
  • The implications of disabling allowJs means that imported modules from *.jsx files are implied to have an any type. Hence, we need noImplicitAny to be false.
@lynnagara
lynnagara
commented
Jul 19, 2019

Copy link


Copy Markdown

Member
Deployed to staging, everything looks fine
@lynnagara
lynnagara
merged commit 7823ddb
into
masterJul 22, 2019
@lynnagara
lynnagara
deleted the

typescript

branch
July 22, 2019 19:19
HazAT added a commit
that referenced
this pull request
Jul 23, 2019
@HazAT
Merge branch 'master' into feat/apm-v2
8d62f17
* master: (25 commits)
ref(onboarding): Fix install promprt URL (#14106)
fix(app-platform): Allow GET requests for published apps (#14109)
feat: Update Group.get_latest_event to use Snuba event (#14039)
ref(onboarding): Rename wizardNew -> onboarding (#14104)
feat(apm): Update props to address proptype warnings for new transaction attributes (SEN-800) (#14040)
ref(ui): Move and codesplit `ProjectPlugins` (#13952)
feat(typescript): Add TypeScript compatibility (#13786)
ref(templates): Remove unused content block default (#14090)
ref(less): Remove unused admin.less (#14097)
ref(onobarding): Remove old onboarding experience (#14066)
fix(ui) Fix missing conditions in tag bars (#14063)
ref(suspect-commits): Add hook (#14057)
ref(frontend): Segment frontend web urls (#14096)
feat(suspect-commits): Add analytics events (#14080)
feat(servicehooks): Update servicehook URLs (#14093)
license: Remove license headers (#14095)
ref(templates): Remove unused account_nav (#14091)
fix: Disable transaction events in store (#14088)
fix(InstallWizard): Fix exception when InstallWizard completed (#14092)
ref(admin): Fix thrashing on stat charts (#14094)
...
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 20, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@dasheddasheddashed approved these changes
@lynnagaralynnagaralynnagara approved these changes

Assignees

@HazATHazAT
@lynnagaralynnagara

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@HazAT@dashed@lynnagara

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

Skip to content
feat(typescript): Add TypeScript compatibility - #13786
Merged
lynnagara merged 44 commits into
masterfrom 
typescript
Jul 22, 2019
Merged
feat(typescript): Add TypeScript compatibility#13786
lynnagara merged 44 commits into
masterfrom 
typescript
Conversation
@HazAT
@HazATHazAT
commented
Jun 24, 2019•

edited by lynnagara

Loading

Copy link


Copy Markdown

Member
This PR is intended to serve as a demonstration for how we could write TypeScript code in Sentry. It migrates the following code to TypeScript:
  • all Discover functionality
  • some utility functions
It depends on:
@dashed
dashed
commented
Jun 24, 2019•

edited

Loading

Copy link


Copy Markdown

Member
At the time of this message, Typescript doesn't support optional chaining proposal (https://github.com/tc39/proposal-optional-chaining), which is stage 2 microsoft/TypeScript#16 .
Some suggestions:
@dashed
dashed
commented
Jun 24, 2019

Copy link


Copy Markdown

Member
tslint is going to be deprecated:
@lynnagara
lynnagaraforce-pushed
the
typescript
branch
2 times, most recently
from
e38d0db to
289b539CompareJune 24, 2019 20:39
lynnagara
lynnagara
reviewed
Jun 24, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/aggregations/index.tsx
Outdated

lynnagara added a commit
that referenced
this pull request
Jun 24, 2019
@lynnagara
ref: Convert app.jsx to ES6
9ba73d0
Uses withConfig HoC instead of connecting to the ConfigStore directly
This is required for #13786
@lynnagaralynnagara
mentioned this pull request
Jun 24, 2019
 Merged

lynnagara added a commit
that referenced
this pull request
Jun 24, 2019
@lynnagara
ref: Remove optional chaining
4c9eba2
This is required for #13786
@lynnagaralynnagara
mentioned this pull request
Jun 25, 2019
 Merged

dashed
dashed
reviewed
Jun 25, 2019
View reviewed changes
Comment thread.eslintrc.js
 '@typescript-eslint/no-unused-vars': 'off',
 },
 },
 ],
@dasheddashedJun 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@typescript-eslint/no-unused-vars rule was causing issues for imported types. see: typescript-eslint/typescript-eslint#363
I've added this as per https://43081j.com/2019/02/using-eslint-with-typescript
dashed
dashed
reviewed
Jun 25, 2019
View reviewed changes
Comment thread.eslintrc.js
Outdated

@dasheddashedJun 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Caveat: jsx-eslint/eslint-plugin-react#2066
@dasheddashed
added
the 
WIP

label
Jun 25, 2019
lynnagara added a commit
that referenced
this pull request
Jun 25, 2019
@lynnagara
ref: Convert app.jsx to ES6 (#13798)
634c2a7
Uses withConfig HoC instead of connecting to the ConfigStore directly
This is required for #13786
lynnagara added a commit
that referenced
this pull request
Jun 25, 2019
@lynnagara
ref: Remove optional chaining
f423a42
This is required for #13786
pullBot
pushed a commit
to Mattlk13/sentry-1
that referenced
this pull request
Jun 25, 2019
@lynnagara
ref: Remove optional chaining (getsentry#13800)
ff3c330
This is required for getsentry#13786
@lynnagara
lynnagaraforce-pushed
the
typescript
branch
2 times, most recently
from
66506af to
aac7d03CompareJune 25, 2019 20:01
@lynnagaralynnagara
mentioned this pull request
Jun 25, 2019
 Merged

lynnagara
lynnagara
reviewed
Jun 27, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/queryBuilder.tsx
 const type = SPECIAL_TAGS[tags.tags_key] || 'string';
 .filter((tag: TagData) => !HIDDEN_TAGS.includes(tag.tags_key))
 .map((tag: TagData) => {
 const type = SPECIAL_TAGS[tag.tags_key] || 'string';
@lynnagaralynnagaraJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This was a good bug, TypeScript made it immediately obvious
lynnagara
lynnagara
reviewed
Jun 27, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/discover.tsx
@@ -409,7 +429,6 @@ export default class OrganizationDiscover extends React.Component {
 isFetchingQuery={isFetchingQuery}
 onUpdateField={this.updateField}
 onRunQuery={this.runQuery}
 onReset={this.reset}
@lynnagaralynnagaraJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

👍 for errors on unused props
@lynnagaralynnagara
removed
the 
WIP

label
Jun 28, 2019
@lynnagaralynnagara
changed the title
[WIP] feat: Add typescript compatibilityfeat(typescript): Add TypeScript compatibilityJun 28, 2019
lynnagara
lynnagara
reviewed
Jul 1, 2019
View reviewed changes
Comment threadpackage.json
 "@sentry/typescript": "^5.3.0",
 "@types/lodash": "^4.14.134",
 "@types/react-dom": "^16.8.4",
 "@types/moment-timezone": "^0.5.12",
@lynnagaralynnagaraJul 1, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@HazAT Do you know if/why these need to be dependencies? Would be really nice to move these to devDependencies?
@HazATHazATJul 10, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sorry for the late reply. I thought we need this since we do different stuff if you call
make develop or yarn install.
make develop apparently on installs dependencies whereby yarn install installs all.
So if you do a prod build it would fail because of missing types.
@lynnagaralynnagaraJul 10, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Gotcha, thanks for the explanation
dashed
dashed
approved these changes
Jul 10, 2019
View reviewed changes
@dasheddashed
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I took the liberty to rebase this branch against master, and looked through the PR.
Looks good to me 👍
dashed
dashed
reviewed
Jul 19, 2019
View reviewed changes
Comment threadtsconfig.json
 "declarationMap": false,
 "allowJs": true,
 "allowJs": false,
 "noImplicitAny": false,
@dasheddashedJul 19, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@HazAT@lynnagara There are issues with importing React components from *.jsx files, and I don't think redeclaring them as const FooBarAny: any = FooBar; is the right approach to migrate Sentry to TypeScript in the long-term.
Since this file is extending @sentry/typescript/tsconfig.json (https://github.com/getsentry/sentry-javascript/blob/5d670a7781b6e2144809c002e9332e2cc9dd239c/packages/typescript/tsconfig.json), I'm proposing to provide the following overrides that are specific for this project:
 "allowJs": false,
"noImplicitAny": false,

The behavioural changes are as follows:
  • Setting allowJs to be false prevents the TypeScript engine from processing *.jsx files and as well as inferring types of imported js code. This resolves cryptic proptype type issues for React components sourced in *.jsx files .
  • The implications of disabling allowJs means that imported modules from *.jsx files are implied to have an any type. Hence, we need noImplicitAny to be false.
@lynnagara
lynnagara
commented
Jul 19, 2019

Copy link


Copy Markdown

Member
Deployed to staging, everything looks fine
@lynnagara
lynnagara
merged commit 7823ddb
into
masterJul 22, 2019
@lynnagara
lynnagara
deleted the

typescript

branch
July 22, 2019 19:19
HazAT added a commit
that referenced
this pull request
Jul 23, 2019
@HazAT
Merge branch 'master' into feat/apm-v2
8d62f17
* master: (25 commits)
ref(onboarding): Fix install promprt URL (#14106)
fix(app-platform): Allow GET requests for published apps (#14109)
feat: Update Group.get_latest_event to use Snuba event (#14039)
ref(onboarding): Rename wizardNew -> onboarding (#14104)
feat(apm): Update props to address proptype warnings for new transaction attributes (SEN-800) (#14040)
ref(ui): Move and codesplit `ProjectPlugins` (#13952)
feat(typescript): Add TypeScript compatibility (#13786)
ref(templates): Remove unused content block default (#14090)
ref(less): Remove unused admin.less (#14097)
ref(onobarding): Remove old onboarding experience (#14066)
fix(ui) Fix missing conditions in tag bars (#14063)
ref(suspect-commits): Add hook (#14057)
ref(frontend): Segment frontend web urls (#14096)
feat(suspect-commits): Add analytics events (#14080)
feat(servicehooks): Update servicehook URLs (#14093)
license: Remove license headers (#14095)
ref(templates): Remove unused account_nav (#14091)
fix: Disable transaction events in store (#14088)
fix(InstallWizard): Fix exception when InstallWizard completed (#14092)
ref(admin): Fix thrashing on stat charts (#14094)
...
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 20, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@dasheddasheddashed approved these changes
@lynnagaralynnagaralynnagara approved these changes

Assignees

@HazATHazAT
@lynnagaralynnagara

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@HazAT@dashed@lynnagara

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
feat(typescript): Add TypeScript compatibility - #13786
Merged
lynnagara merged 44 commits into
masterfrom 
typescript
Jul 22, 2019
Merged
feat(typescript): Add TypeScript compatibility#13786
lynnagara merged 44 commits into
masterfrom 
typescript
Conversation
@HazAT
@HazATHazAT
commented
Jun 24, 2019•

edited by lynnagara

Loading

Copy link


Copy Markdown

Member
This PR is intended to serve as a demonstration for how we could write TypeScript code in Sentry. It migrates the following code to TypeScript:
  • all Discover functionality
  • some utility functions
It depends on:
@dashed
dashed
commented
Jun 24, 2019•

edited

Loading

Copy link


Copy Markdown

Member
At the time of this message, Typescript doesn't support optional chaining proposal (https://github.com/tc39/proposal-optional-chaining), which is stage 2 microsoft/TypeScript#16 .
Some suggestions:
@dashed
dashed
commented
Jun 24, 2019

Copy link


Copy Markdown

Member
tslint is going to be deprecated:
@lynnagara
lynnagaraforce-pushed
the
typescript
branch
2 times, most recently
from
e38d0db to
289b539CompareJune 24, 2019 20:39
lynnagara
lynnagara
reviewed
Jun 24, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/aggregations/index.tsx
Outdated

lynnagara added a commit
that referenced
this pull request
Jun 24, 2019
@lynnagara
ref: Convert app.jsx to ES6
9ba73d0
Uses withConfig HoC instead of connecting to the ConfigStore directly
This is required for #13786
@lynnagaralynnagara
mentioned this pull request
Jun 24, 2019
 Merged

lynnagara added a commit
that referenced
this pull request
Jun 24, 2019
@lynnagara
ref: Remove optional chaining
4c9eba2
This is required for #13786
@lynnagaralynnagara
mentioned this pull request
Jun 25, 2019
 Merged

dashed
dashed
reviewed
Jun 25, 2019
View reviewed changes
Comment thread.eslintrc.js
 '@typescript-eslint/no-unused-vars': 'off',
 },
 },
 ],
@dasheddashedJun 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@typescript-eslint/no-unused-vars rule was causing issues for imported types. see: typescript-eslint/typescript-eslint#363
I've added this as per https://43081j.com/2019/02/using-eslint-with-typescript
dashed
dashed
reviewed
Jun 25, 2019
View reviewed changes
Comment thread.eslintrc.js
Outdated

@dasheddashedJun 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Caveat: jsx-eslint/eslint-plugin-react#2066
@dasheddashed
added
the 
WIP

label
Jun 25, 2019
lynnagara added a commit
that referenced
this pull request
Jun 25, 2019
@lynnagara
ref: Convert app.jsx to ES6 (#13798)
634c2a7
Uses withConfig HoC instead of connecting to the ConfigStore directly
This is required for #13786
lynnagara added a commit
that referenced
this pull request
Jun 25, 2019
@lynnagara
ref: Remove optional chaining
f423a42
This is required for #13786
pullBot
pushed a commit
to Mattlk13/sentry-1
that referenced
this pull request
Jun 25, 2019
@lynnagara
ref: Remove optional chaining (getsentry#13800)
ff3c330
This is required for getsentry#13786
@lynnagara
lynnagaraforce-pushed
the
typescript
branch
2 times, most recently
from
66506af to
aac7d03CompareJune 25, 2019 20:01
@lynnagaralynnagara
mentioned this pull request
Jun 25, 2019
 Merged

lynnagara
lynnagara
reviewed
Jun 27, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/queryBuilder.tsx
 const type = SPECIAL_TAGS[tags.tags_key] || 'string';
 .filter((tag: TagData) => !HIDDEN_TAGS.includes(tag.tags_key))
 .map((tag: TagData) => {
 const type = SPECIAL_TAGS[tag.tags_key] || 'string';
@lynnagaralynnagaraJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This was a good bug, TypeScript made it immediately obvious
lynnagara
lynnagara
reviewed
Jun 27, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/discover.tsx
@@ -409,7 +429,6 @@ export default class OrganizationDiscover extends React.Component {
 isFetchingQuery={isFetchingQuery}
 onUpdateField={this.updateField}
 onRunQuery={this.runQuery}
 onReset={this.reset}
@lynnagaralynnagaraJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

👍 for errors on unused props
@lynnagaralynnagara
removed
the 
WIP

label
Jun 28, 2019
@lynnagaralynnagara
changed the title
[WIP] feat: Add typescript compatibilityfeat(typescript): Add TypeScript compatibilityJun 28, 2019
lynnagara
lynnagara
reviewed
Jul 1, 2019
View reviewed changes
Comment threadpackage.json
 "@sentry/typescript": "^5.3.0",
 "@types/lodash": "^4.14.134",
 "@types/react-dom": "^16.8.4",
 "@types/moment-timezone": "^0.5.12",
@lynnagaralynnagaraJul 1, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@HazAT Do you know if/why these need to be dependencies? Would be really nice to move these to devDependencies?
@HazATHazATJul 10, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sorry for the late reply. I thought we need this since we do different stuff if you call
make develop or yarn install.
make develop apparently on installs dependencies whereby yarn install installs all.
So if you do a prod build it would fail because of missing types.
@lynnagaralynnagaraJul 10, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Gotcha, thanks for the explanation
dashed
dashed
approved these changes
Jul 10, 2019
View reviewed changes
@dasheddashed
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I took the liberty to rebase this branch against master, and looked through the PR.
Looks good to me 👍
dashed
dashed
reviewed
Jul 19, 2019
View reviewed changes
Comment threadtsconfig.json
 "declarationMap": false,
 "allowJs": true,
 "allowJs": false,
 "noImplicitAny": false,
@dasheddashedJul 19, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@HazAT@lynnagara There are issues with importing React components from *.jsx files, and I don't think redeclaring them as const FooBarAny: any = FooBar; is the right approach to migrate Sentry to TypeScript in the long-term.
Since this file is extending @sentry/typescript/tsconfig.json (https://github.com/getsentry/sentry-javascript/blob/5d670a7781b6e2144809c002e9332e2cc9dd239c/packages/typescript/tsconfig.json), I'm proposing to provide the following overrides that are specific for this project:
 "allowJs": false,
"noImplicitAny": false,

The behavioural changes are as follows:
  • Setting allowJs to be false prevents the TypeScript engine from processing *.jsx files and as well as inferring types of imported js code. This resolves cryptic proptype type issues for React components sourced in *.jsx files .
  • The implications of disabling allowJs means that imported modules from *.jsx files are implied to have an any type. Hence, we need noImplicitAny to be false.
@lynnagara
lynnagara
commented
Jul 19, 2019

Copy link


Copy Markdown

Member
Deployed to staging, everything looks fine
@lynnagara
lynnagara
merged commit 7823ddb
into
masterJul 22, 2019
@lynnagara
lynnagara
deleted the

typescript

branch
July 22, 2019 19:19
HazAT added a commit
that referenced
this pull request
Jul 23, 2019
@HazAT
Merge branch 'master' into feat/apm-v2
8d62f17
* master: (25 commits)
ref(onboarding): Fix install promprt URL (#14106)
fix(app-platform): Allow GET requests for published apps (#14109)
feat: Update Group.get_latest_event to use Snuba event (#14039)
ref(onboarding): Rename wizardNew -> onboarding (#14104)
feat(apm): Update props to address proptype warnings for new transaction attributes (SEN-800) (#14040)
ref(ui): Move and codesplit `ProjectPlugins` (#13952)
feat(typescript): Add TypeScript compatibility (#13786)
ref(templates): Remove unused content block default (#14090)
ref(less): Remove unused admin.less (#14097)
ref(onobarding): Remove old onboarding experience (#14066)
fix(ui) Fix missing conditions in tag bars (#14063)
ref(suspect-commits): Add hook (#14057)
ref(frontend): Segment frontend web urls (#14096)
feat(suspect-commits): Add analytics events (#14080)
feat(servicehooks): Update servicehook URLs (#14093)
license: Remove license headers (#14095)
ref(templates): Remove unused account_nav (#14091)
fix: Disable transaction events in store (#14088)
fix(InstallWizard): Fix exception when InstallWizard completed (#14092)
ref(admin): Fix thrashing on stat charts (#14094)
...
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 20, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@dasheddasheddashed approved these changes
@lynnagaralynnagaralynnagara approved these changes

Assignees

@HazATHazAT
@lynnagaralynnagara

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@HazAT@dashed@lynnagara

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(typescript): Add TypeScript compatibility - #13786
Merged
lynnagara merged 44 commits into
masterfrom 
typescript
Jul 22, 2019
Merged
feat(typescript): Add TypeScript compatibility#13786
lynnagara merged 44 commits into
masterfrom 
typescript
Conversation
@HazAT
@HazATHazAT
commented
Jun 24, 2019•

edited by lynnagara

Loading

Copy link


Copy Markdown

Member
This PR is intended to serve as a demonstration for how we could write TypeScript code in Sentry. It migrates the following code to TypeScript:
  • all Discover functionality
  • some utility functions
It depends on:
@dashed
dashed
commented
Jun 24, 2019•

edited

Loading

Copy link


Copy Markdown

Member
At the time of this message, Typescript doesn't support optional chaining proposal (https://github.com/tc39/proposal-optional-chaining), which is stage 2 microsoft/TypeScript#16 .
Some suggestions:
@dashed
dashed
commented
Jun 24, 2019

Copy link


Copy Markdown

Member
tslint is going to be deprecated:
@lynnagara
lynnagaraforce-pushed
the
typescript
branch
2 times, most recently
from
e38d0db to
289b539CompareJune 24, 2019 20:39
lynnagara
lynnagara
reviewed
Jun 24, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/aggregations/index.tsx
Outdated

lynnagara added a commit
that referenced
this pull request
Jun 24, 2019
@lynnagara
ref: Convert app.jsx to ES6
9ba73d0
Uses withConfig HoC instead of connecting to the ConfigStore directly
This is required for #13786
@lynnagaralynnagara
mentioned this pull request
Jun 24, 2019
 Merged

lynnagara added a commit
that referenced
this pull request
Jun 24, 2019
@lynnagara
ref: Remove optional chaining
4c9eba2
This is required for #13786
@lynnagaralynnagara
mentioned this pull request
Jun 25, 2019
 Merged

dashed
dashed
reviewed
Jun 25, 2019
View reviewed changes
Comment thread.eslintrc.js
 '@typescript-eslint/no-unused-vars': 'off',
 },
 },
 ],
@dasheddashedJun 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@typescript-eslint/no-unused-vars rule was causing issues for imported types. see: typescript-eslint/typescript-eslint#363
I've added this as per https://43081j.com/2019/02/using-eslint-with-typescript
dashed
dashed
reviewed
Jun 25, 2019
View reviewed changes
Comment thread.eslintrc.js
Outdated

@dasheddashedJun 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Caveat: jsx-eslint/eslint-plugin-react#2066
@dasheddashed
added
the 
WIP

label
Jun 25, 2019
lynnagara added a commit
that referenced
this pull request
Jun 25, 2019
@lynnagara
ref: Convert app.jsx to ES6 (#13798)
634c2a7
Uses withConfig HoC instead of connecting to the ConfigStore directly
This is required for #13786
lynnagara added a commit
that referenced
this pull request
Jun 25, 2019
@lynnagara
ref: Remove optional chaining
f423a42
This is required for #13786
pullBot
pushed a commit
to Mattlk13/sentry-1
that referenced
this pull request
Jun 25, 2019
@lynnagara
ref: Remove optional chaining (getsentry#13800)
ff3c330
This is required for getsentry#13786
@lynnagara
lynnagaraforce-pushed
the
typescript
branch
2 times, most recently
from
66506af to
aac7d03CompareJune 25, 2019 20:01
@lynnagaralynnagara
mentioned this pull request
Jun 25, 2019
 Merged

lynnagara
lynnagara
reviewed
Jun 27, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/queryBuilder.tsx
 const type = SPECIAL_TAGS[tags.tags_key] || 'string';
 .filter((tag: TagData) => !HIDDEN_TAGS.includes(tag.tags_key))
 .map((tag: TagData) => {
 const type = SPECIAL_TAGS[tag.tags_key] || 'string';
@lynnagaralynnagaraJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This was a good bug, TypeScript made it immediately obvious
lynnagara
lynnagara
reviewed
Jun 27, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/discover.tsx
@@ -409,7 +429,6 @@ export default class OrganizationDiscover extends React.Component {
 isFetchingQuery={isFetchingQuery}
 onUpdateField={this.updateField}
 onRunQuery={this.runQuery}
 onReset={this.reset}
@lynnagaralynnagaraJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

👍 for errors on unused props
@lynnagaralynnagara
removed
the 
WIP

label
Jun 28, 2019
@lynnagaralynnagara
changed the title
[WIP] feat: Add typescript compatibilityfeat(typescript): Add TypeScript compatibilityJun 28, 2019
lynnagara
lynnagara
reviewed
Jul 1, 2019
View reviewed changes
Comment threadpackage.json
 "@sentry/typescript": "^5.3.0",
 "@types/lodash": "^4.14.134",
 "@types/react-dom": "^16.8.4",
 "@types/moment-timezone": "^0.5.12",
@lynnagaralynnagaraJul 1, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@HazAT Do you know if/why these need to be dependencies? Would be really nice to move these to devDependencies?
@HazATHazATJul 10, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sorry for the late reply. I thought we need this since we do different stuff if you call
make develop or yarn install.
make develop apparently on installs dependencies whereby yarn install installs all.
So if you do a prod build it would fail because of missing types.
@lynnagaralynnagaraJul 10, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Gotcha, thanks for the explanation
dashed
dashed
approved these changes
Jul 10, 2019
View reviewed changes
@dasheddashed
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I took the liberty to rebase this branch against master, and looked through the PR.
Looks good to me 👍
dashed
dashed
reviewed
Jul 19, 2019
View reviewed changes
Comment threadtsconfig.json
 "declarationMap": false,
 "allowJs": true,
 "allowJs": false,
 "noImplicitAny": false,
@dasheddashedJul 19, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@HazAT@lynnagara There are issues with importing React components from *.jsx files, and I don't think redeclaring them as const FooBarAny: any = FooBar; is the right approach to migrate Sentry to TypeScript in the long-term.
Since this file is extending @sentry/typescript/tsconfig.json (https://github.com/getsentry/sentry-javascript/blob/5d670a7781b6e2144809c002e9332e2cc9dd239c/packages/typescript/tsconfig.json), I'm proposing to provide the following overrides that are specific for this project:
 "allowJs": false,
"noImplicitAny": false,

The behavioural changes are as follows:
  • Setting allowJs to be false prevents the TypeScript engine from processing *.jsx files and as well as inferring types of imported js code. This resolves cryptic proptype type issues for React components sourced in *.jsx files .
  • The implications of disabling allowJs means that imported modules from *.jsx files are implied to have an any type. Hence, we need noImplicitAny to be false.
@lynnagara
lynnagara
commented
Jul 19, 2019

Copy link


Copy Markdown

Member
Deployed to staging, everything looks fine
@lynnagara
lynnagara
merged commit 7823ddb
into
masterJul 22, 2019
@lynnagara
lynnagara
deleted the

typescript

branch
July 22, 2019 19:19
HazAT added a commit
that referenced
this pull request
Jul 23, 2019
@HazAT
Merge branch 'master' into feat/apm-v2
8d62f17
* master: (25 commits)
ref(onboarding): Fix install promprt URL (#14106)
fix(app-platform): Allow GET requests for published apps (#14109)
feat: Update Group.get_latest_event to use Snuba event (#14039)
ref(onboarding): Rename wizardNew -> onboarding (#14104)
feat(apm): Update props to address proptype warnings for new transaction attributes (SEN-800) (#14040)
ref(ui): Move and codesplit `ProjectPlugins` (#13952)
feat(typescript): Add TypeScript compatibility (#13786)
ref(templates): Remove unused content block default (#14090)
ref(less): Remove unused admin.less (#14097)
ref(onobarding): Remove old onboarding experience (#14066)
fix(ui) Fix missing conditions in tag bars (#14063)
ref(suspect-commits): Add hook (#14057)
ref(frontend): Segment frontend web urls (#14096)
feat(suspect-commits): Add analytics events (#14080)
feat(servicehooks): Update servicehook URLs (#14093)
license: Remove license headers (#14095)
ref(templates): Remove unused account_nav (#14091)
fix: Disable transaction events in store (#14088)
fix(InstallWizard): Fix exception when InstallWizard completed (#14092)
ref(admin): Fix thrashing on stat charts (#14094)
...
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 20, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@dasheddasheddashed approved these changes
@lynnagaralynnagaralynnagara approved these changes

Assignees

@HazATHazAT
@lynnagaralynnagara

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@HazAT@dashed@lynnagara

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(typescript): Add TypeScript compatibility - #13786
Merged
lynnagara merged 44 commits into
masterfrom 
typescript
Jul 22, 2019
Merged
feat(typescript): Add TypeScript compatibility#13786
lynnagara merged 44 commits into
masterfrom 
typescript
Conversation
@HazAT
@HazATHazAT
commented
Jun 24, 2019•

edited by lynnagara

Loading

Copy link


Copy Markdown

Member
This PR is intended to serve as a demonstration for how we could write TypeScript code in Sentry. It migrates the following code to TypeScript:
  • all Discover functionality
  • some utility functions
It depends on:
@dashed
dashed
commented
Jun 24, 2019•

edited

Loading

Copy link


Copy Markdown

Member
At the time of this message, Typescript doesn't support optional chaining proposal (https://github.com/tc39/proposal-optional-chaining), which is stage 2 microsoft/TypeScript#16 .
Some suggestions:
@dashed
dashed
commented
Jun 24, 2019

Copy link


Copy Markdown

Member
tslint is going to be deprecated:
@lynnagara
lynnagaraforce-pushed
the
typescript
branch
2 times, most recently
from
e38d0db to
289b539CompareJune 24, 2019 20:39
lynnagara
lynnagara
reviewed
Jun 24, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/aggregations/index.tsx
Outdated

lynnagara added a commit
that referenced
this pull request
Jun 24, 2019
@lynnagara
ref: Convert app.jsx to ES6
9ba73d0
Uses withConfig HoC instead of connecting to the ConfigStore directly
This is required for #13786
@lynnagaralynnagara
mentioned this pull request
Jun 24, 2019
 Merged

lynnagara added a commit
that referenced
this pull request
Jun 24, 2019
@lynnagara
ref: Remove optional chaining
4c9eba2
This is required for #13786
@lynnagaralynnagara
mentioned this pull request
Jun 25, 2019
 Merged

dashed
dashed
reviewed
Jun 25, 2019
View reviewed changes
Comment thread.eslintrc.js
 '@typescript-eslint/no-unused-vars': 'off',
 },
 },
 ],
@dasheddashedJun 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@typescript-eslint/no-unused-vars rule was causing issues for imported types. see: typescript-eslint/typescript-eslint#363
I've added this as per https://43081j.com/2019/02/using-eslint-with-typescript
dashed
dashed
reviewed
Jun 25, 2019
View reviewed changes
Comment thread.eslintrc.js
Outdated

@dasheddashedJun 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Caveat: jsx-eslint/eslint-plugin-react#2066
@dasheddashed
added
the 
WIP

label
Jun 25, 2019
lynnagara added a commit
that referenced
this pull request
Jun 25, 2019
@lynnagara
ref: Convert app.jsx to ES6 (#13798)
634c2a7
Uses withConfig HoC instead of connecting to the ConfigStore directly
This is required for #13786
lynnagara added a commit
that referenced
this pull request
Jun 25, 2019
@lynnagara
ref: Remove optional chaining
f423a42
This is required for #13786
pullBot
pushed a commit
to Mattlk13/sentry-1
that referenced
this pull request
Jun 25, 2019
@lynnagara
ref: Remove optional chaining (getsentry#13800)
ff3c330
This is required for getsentry#13786
@lynnagara
lynnagaraforce-pushed
the
typescript
branch
2 times, most recently
from
66506af to
aac7d03CompareJune 25, 2019 20:01
@lynnagaralynnagara
mentioned this pull request
Jun 25, 2019
 Merged

lynnagara
lynnagara
reviewed
Jun 27, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/queryBuilder.tsx
 const type = SPECIAL_TAGS[tags.tags_key] || 'string';
 .filter((tag: TagData) => !HIDDEN_TAGS.includes(tag.tags_key))
 .map((tag: TagData) => {
 const type = SPECIAL_TAGS[tag.tags_key] || 'string';
@lynnagaralynnagaraJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This was a good bug, TypeScript made it immediately obvious
lynnagara
lynnagara
reviewed
Jun 27, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/discover.tsx
@@ -409,7 +429,6 @@ export default class OrganizationDiscover extends React.Component {
 isFetchingQuery={isFetchingQuery}
 onUpdateField={this.updateField}
 onRunQuery={this.runQuery}
 onReset={this.reset}
@lynnagaralynnagaraJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

👍 for errors on unused props
@lynnagaralynnagara
removed
the 
WIP

label
Jun 28, 2019
@lynnagaralynnagara
changed the title
[WIP] feat: Add typescript compatibilityfeat(typescript): Add TypeScript compatibilityJun 28, 2019
lynnagara
lynnagara
reviewed
Jul 1, 2019
View reviewed changes
Comment threadpackage.json
 "@sentry/typescript": "^5.3.0",
 "@types/lodash": "^4.14.134",
 "@types/react-dom": "^16.8.4",
 "@types/moment-timezone": "^0.5.12",
@lynnagaralynnagaraJul 1, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@HazAT Do you know if/why these need to be dependencies? Would be really nice to move these to devDependencies?
@HazATHazATJul 10, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sorry for the late reply. I thought we need this since we do different stuff if you call
make develop or yarn install.
make develop apparently on installs dependencies whereby yarn install installs all.
So if you do a prod build it would fail because of missing types.
@lynnagaralynnagaraJul 10, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Gotcha, thanks for the explanation
dashed
dashed
approved these changes
Jul 10, 2019
View reviewed changes
@dasheddashed
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I took the liberty to rebase this branch against master, and looked through the PR.
Looks good to me 👍
dashed
dashed
reviewed
Jul 19, 2019
View reviewed changes
Comment threadtsconfig.json
 "declarationMap": false,
 "allowJs": true,
 "allowJs": false,
 "noImplicitAny": false,
@dasheddashedJul 19, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@HazAT@lynnagara There are issues with importing React components from *.jsx files, and I don't think redeclaring them as const FooBarAny: any = FooBar; is the right approach to migrate Sentry to TypeScript in the long-term.
Since this file is extending @sentry/typescript/tsconfig.json (https://github.com/getsentry/sentry-javascript/blob/5d670a7781b6e2144809c002e9332e2cc9dd239c/packages/typescript/tsconfig.json), I'm proposing to provide the following overrides that are specific for this project:
 "allowJs": false,
"noImplicitAny": false,

The behavioural changes are as follows:
  • Setting allowJs to be false prevents the TypeScript engine from processing *.jsx files and as well as inferring types of imported js code. This resolves cryptic proptype type issues for React components sourced in *.jsx files .
  • The implications of disabling allowJs means that imported modules from *.jsx files are implied to have an any type. Hence, we need noImplicitAny to be false.
@lynnagara
lynnagara
commented
Jul 19, 2019

Copy link


Copy Markdown

Member
Deployed to staging, everything looks fine
@lynnagara
lynnagara
merged commit 7823ddb
into
masterJul 22, 2019
@lynnagara
lynnagara
deleted the

typescript

branch
July 22, 2019 19:19
HazAT added a commit
that referenced
this pull request
Jul 23, 2019
@HazAT
Merge branch 'master' into feat/apm-v2
8d62f17
* master: (25 commits)
ref(onboarding): Fix install promprt URL (#14106)
fix(app-platform): Allow GET requests for published apps (#14109)
feat: Update Group.get_latest_event to use Snuba event (#14039)
ref(onboarding): Rename wizardNew -> onboarding (#14104)
feat(apm): Update props to address proptype warnings for new transaction attributes (SEN-800) (#14040)
ref(ui): Move and codesplit `ProjectPlugins` (#13952)
feat(typescript): Add TypeScript compatibility (#13786)
ref(templates): Remove unused content block default (#14090)
ref(less): Remove unused admin.less (#14097)
ref(onobarding): Remove old onboarding experience (#14066)
fix(ui) Fix missing conditions in tag bars (#14063)
ref(suspect-commits): Add hook (#14057)
ref(frontend): Segment frontend web urls (#14096)
feat(suspect-commits): Add analytics events (#14080)
feat(servicehooks): Update servicehook URLs (#14093)
license: Remove license headers (#14095)
ref(templates): Remove unused account_nav (#14091)
fix: Disable transaction events in store (#14088)
fix(InstallWizard): Fix exception when InstallWizard completed (#14092)
ref(admin): Fix thrashing on stat charts (#14094)
...
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 20, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@dasheddasheddashed approved these changes
@lynnagaralynnagaralynnagara approved these changes

Assignees

@HazATHazAT
@lynnagaralynnagara

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@HazAT@dashed@lynnagara

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

Skip to content
feat(typescript): Add TypeScript compatibility - #13786
Merged
lynnagara merged 44 commits into
masterfrom 
typescript
Jul 22, 2019
Merged
feat(typescript): Add TypeScript compatibility#13786
lynnagara merged 44 commits into
masterfrom 
typescript
Conversation
@HazAT
@HazATHazAT
commented
Jun 24, 2019•

edited by lynnagara

Loading

Copy link


Copy Markdown

Member
This PR is intended to serve as a demonstration for how we could write TypeScript code in Sentry. It migrates the following code to TypeScript:
  • all Discover functionality
  • some utility functions
It depends on:
@dashed
dashed
commented
Jun 24, 2019•

edited

Loading

Copy link


Copy Markdown

Member
At the time of this message, Typescript doesn't support optional chaining proposal (https://github.com/tc39/proposal-optional-chaining), which is stage 2 microsoft/TypeScript#16 .
Some suggestions:
@dashed
dashed
commented
Jun 24, 2019

Copy link


Copy Markdown

Member
tslint is going to be deprecated:
@lynnagara
lynnagaraforce-pushed
the
typescript
branch
2 times, most recently
from
e38d0db to
289b539CompareJune 24, 2019 20:39
lynnagara
lynnagara
reviewed
Jun 24, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/aggregations/index.tsx
Outdated

lynnagara added a commit
that referenced
this pull request
Jun 24, 2019
@lynnagara
ref: Convert app.jsx to ES6
9ba73d0
Uses withConfig HoC instead of connecting to the ConfigStore directly
This is required for #13786
@lynnagaralynnagara
mentioned this pull request
Jun 24, 2019
 Merged

lynnagara added a commit
that referenced
this pull request
Jun 24, 2019
@lynnagara
ref: Remove optional chaining
4c9eba2
This is required for #13786
@lynnagaralynnagara
mentioned this pull request
Jun 25, 2019
 Merged

dashed
dashed
reviewed
Jun 25, 2019
View reviewed changes
Comment thread.eslintrc.js
 '@typescript-eslint/no-unused-vars': 'off',
 },
 },
 ],
@dasheddashedJun 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@typescript-eslint/no-unused-vars rule was causing issues for imported types. see: typescript-eslint/typescript-eslint#363
I've added this as per https://43081j.com/2019/02/using-eslint-with-typescript
dashed
dashed
reviewed
Jun 25, 2019
View reviewed changes
Comment thread.eslintrc.js
Outdated

@dasheddashedJun 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Caveat: jsx-eslint/eslint-plugin-react#2066
@dasheddashed
added
the 
WIP

label
Jun 25, 2019
lynnagara added a commit
that referenced
this pull request
Jun 25, 2019
@lynnagara
ref: Convert app.jsx to ES6 (#13798)
634c2a7
Uses withConfig HoC instead of connecting to the ConfigStore directly
This is required for #13786
lynnagara added a commit
that referenced
this pull request
Jun 25, 2019
@lynnagara
ref: Remove optional chaining
f423a42
This is required for #13786
pullBot
pushed a commit
to Mattlk13/sentry-1
that referenced
this pull request
Jun 25, 2019
@lynnagara
ref: Remove optional chaining (getsentry#13800)
ff3c330
This is required for getsentry#13786
@lynnagara
lynnagaraforce-pushed
the
typescript
branch
2 times, most recently
from
66506af to
aac7d03CompareJune 25, 2019 20:01
@lynnagaralynnagara
mentioned this pull request
Jun 25, 2019
 Merged

lynnagara
lynnagara
reviewed
Jun 27, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/queryBuilder.tsx
 const type = SPECIAL_TAGS[tags.tags_key] || 'string';
 .filter((tag: TagData) => !HIDDEN_TAGS.includes(tag.tags_key))
 .map((tag: TagData) => {
 const type = SPECIAL_TAGS[tag.tags_key] || 'string';
@lynnagaralynnagaraJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This was a good bug, TypeScript made it immediately obvious
lynnagara
lynnagara
reviewed
Jun 27, 2019
View reviewed changes
Comment threadsrc/sentry/static/sentry/app/views/organizationDiscover/discover.tsx
@@ -409,7 +429,6 @@ export default class OrganizationDiscover extends React.Component {
 isFetchingQuery={isFetchingQuery}
 onUpdateField={this.updateField}
 onRunQuery={this.runQuery}
 onReset={this.reset}
@lynnagaralynnagaraJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

👍 for errors on unused props
@lynnagaralynnagara
removed
the 
WIP

label
Jun 28, 2019
@lynnagaralynnagara
changed the title
[WIP] feat: Add typescript compatibilityfeat(typescript): Add TypeScript compatibilityJun 28, 2019
lynnagara
lynnagara
reviewed
Jul 1, 2019
View reviewed changes
Comment threadpackage.json
 "@sentry/typescript": "^5.3.0",
 "@types/lodash": "^4.14.134",
 "@types/react-dom": "^16.8.4",
 "@types/moment-timezone": "^0.5.12",
@lynnagaralynnagaraJul 1, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@HazAT Do you know if/why these need to be dependencies? Would be really nice to move these to devDependencies?
@HazATHazATJul 10, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sorry for the late reply. I thought we need this since we do different stuff if you call
make develop or yarn install.
make develop apparently on installs dependencies whereby yarn install installs all.
So if you do a prod build it would fail because of missing types.
@lynnagaralynnagaraJul 10, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Gotcha, thanks for the explanation
dashed
dashed
approved these changes
Jul 10, 2019
View reviewed changes
@dasheddashed
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I took the liberty to rebase this branch against master, and looked through the PR.
Looks good to me 👍
dashed
dashed
reviewed
Jul 19, 2019
View reviewed changes
Comment threadtsconfig.json
 "declarationMap": false,
 "allowJs": true,
 "allowJs": false,
 "noImplicitAny": false,
@dasheddashedJul 19, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@HazAT@lynnagara There are issues with importing React components from *.jsx files, and I don't think redeclaring them as const FooBarAny: any = FooBar; is the right approach to migrate Sentry to TypeScript in the long-term.
Since this file is extending @sentry/typescript/tsconfig.json (https://github.com/getsentry/sentry-javascript/blob/5d670a7781b6e2144809c002e9332e2cc9dd239c/packages/typescript/tsconfig.json), I'm proposing to provide the following overrides that are specific for this project:
 "allowJs": false,
"noImplicitAny": false,

The behavioural changes are as follows:
  • Setting allowJs to be false prevents the TypeScript engine from processing *.jsx files and as well as inferring types of imported js code. This resolves cryptic proptype type issues for React components sourced in *.jsx files .
  • The implications of disabling allowJs means that imported modules from *.jsx files are implied to have an any type. Hence, we need noImplicitAny to be false.
@lynnagara
lynnagara
commented
Jul 19, 2019

Copy link


Copy Markdown

Member
Deployed to staging, everything looks fine
@lynnagara
lynnagara
merged commit 7823ddb
into
masterJul 22, 2019
@lynnagara
lynnagara
deleted the

typescript

branch
July 22, 2019 19:19
HazAT added a commit
that referenced
this pull request
Jul 23, 2019
@HazAT
Merge branch 'master' into feat/apm-v2
8d62f17
* master: (25 commits)
ref(onboarding): Fix install promprt URL (#14106)
fix(app-platform): Allow GET requests for published apps (#14109)
feat: Update Group.get_latest_event to use Snuba event (#14039)
ref(onboarding): Rename wizardNew -> onboarding (#14104)
feat(apm): Update props to address proptype warnings for new transaction attributes (SEN-800) (#14040)
ref(ui): Move and codesplit `ProjectPlugins` (#13952)
feat(typescript): Add TypeScript compatibility (#13786)
ref(templates): Remove unused content block default (#14090)
ref(less): Remove unused admin.less (#14097)
ref(onobarding): Remove old onboarding experience (#14066)
fix(ui) Fix missing conditions in tag bars (#14063)
ref(suspect-commits): Add hook (#14057)
ref(frontend): Segment frontend web urls (#14096)
feat(suspect-commits): Add analytics events (#14080)
feat(servicehooks): Update servicehook URLs (#14093)
license: Remove license headers (#14095)
ref(templates): Remove unused account_nav (#14091)
fix: Disable transaction events in store (#14088)
fix(InstallWizard): Fix exception when InstallWizard completed (#14092)
ref(admin): Fix thrashing on stat charts (#14094)
...
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 20, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@dasheddasheddashed approved these changes
@lynnagaralynnagaralynnagara approved these changes

Assignees

@HazATHazAT
@lynnagaralynnagara

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@HazAT@dashed@lynnagara