Make ScriptElementKind and HighlightSpanKind string enums - #15966

Merged
6 commits merged into
masterfrom
kind
May 23, 2017
Merged

Make ScriptElementKind and HighlightSpanKind string enums#15966
6 commits merged into
masterfrom
kind

Conversation

@ghost

@ghostDeleted user (ghost) commented May 20, 2017

Copy link
Copy Markdown

Note: is technically a change to our public API as many strings are now ScriptElementKinds. And anyone using the API must now have ts@next or they will get compile errors on the string enums -- but presumably people compiling against the ts@next API are compiling using ts@next too.
Required running gulp LKG to get #15486 in.

mhegazy
Mohamed Hegazy (mhegazy) reviewed May 22, 2017
View reviewed changes
Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should also change ScriptElementKindModifier to be an enum.

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and ClassificationTypeNames

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also server.CommandTypes

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and HighlightSpanKind

mhegazy
Mohamed Hegazy (mhegazy) suggested changes May 22, 2017
View reviewed changes
Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and ClassificationTypeNames

Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also server.CommandTypes

Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and HighlightSpanKind

@mhegazy

Mohamed Hegazy (mhegazy) commented May 22, 2017

Copy link
Copy Markdown
Contributor

Also please add a note for this in https://github.com/Microsoft/TypeScript/wiki/API-Breaking-Changes#typescript-24

@mhegazyMohamed Hegazy (mhegazy) added Breaking Change Would introduce errors in existing code Domain: API Relates to the public API for TypeScript labels May 22, 2017
@ghost

Deleted user (ghost) commented May 22, 2017

Copy link
Copy Markdown
Author

Mohamed Hegazy (@mhegazy) Is there any reason to avoid a non-const enum in protocol.ts? We iterate over for (const name in CommandNames) in a unit test (and now CommandNames = CommandTypes).

mhegazy
Mohamed Hegazy (mhegazy) reviewed May 22, 2017
View reviewed changes
Comment threadsrc/server/protocol.ts Outdated
namespace ts.server.protocol {
export namespace CommandTypes {
export type Brace = "brace";
export enum CommandTypes {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should be const enum, since it does not exist at time of building the client against the protocol file.

@ghostDeleted user (ghost)May 22, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We need to iterate over it though -- I guess I could define a CommandTypes[] elsewhere but we'd have to remember to update both places every time a new command is added.

mhegazy
Mohamed Hegazy (mhegazy) suggested changes May 22, 2017
View reviewed changes
Comment threadsrc/server/protocol.ts
export namespace NewLineKind {
export type Crlf = "Crlf";
export type Lf = "Lf";
export const enum NewLineKind {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

missing the initialization here.

@mhegazy

Mohamed Hegazy (mhegazy) commented May 22, 2017

Copy link
Copy Markdown
Contributor

Mohamed Hegazy (@mhegazy) Is there any reason to avoid a non-const enum in protocol.ts? We iterate over for (const name in CommandNames) in a unit test (and now CommandNames = CommandTypes).

yes. protocol.ts compiles down to protocol.d.ts which is then used by the clients to compile against.

so either we make them const in protocol.ts or do that while we put together protocol.d.ts in buildProtocol.js.

@ghost
ghost merged commit 73ee2fe into masterMay 23, 2017
@ghost
ghost deleted the kind branch May 23, 2017 14:19
@microsoftMicrosoft (microsoft) locked and limited conversation to collaborators Jun 14, 2018
This pull request was closed.
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Breaking ChangeWould introduce errors in existing codeDomain: APIRelates to the public API for TypeScript

Projects

None yet

Milestone

TypeScript 2.4

Development

Successfully merging this pull request may close these issues.

2 participants

@mhegazy@msftclas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all \u003cpre\u003e\u003ccode\u003e 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

Make ScriptElementKind and HighlightSpanKind string enums - #15966

Merged
6 commits merged into
masterfrom
kind
May 23, 2017
Merged

Make ScriptElementKind and HighlightSpanKind string enums#15966
6 commits merged into
masterfrom
kind

Conversation

@ghost

@ghostDeleted user (ghost) commented May 20, 2017

Copy link
Copy Markdown

Note: is technically a change to our public API as many strings are now ScriptElementKinds. And anyone using the API must now have ts@next or they will get compile errors on the string enums -- but presumably people compiling against the ts@next API are compiling using ts@next too.
Required running gulp LKG to get #15486 in.

mhegazy
Mohamed Hegazy (mhegazy) reviewed May 22, 2017
View reviewed changes
Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should also change ScriptElementKindModifier to be an enum.

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and ClassificationTypeNames

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also server.CommandTypes

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and HighlightSpanKind

mhegazy
Mohamed Hegazy (mhegazy) suggested changes May 22, 2017
View reviewed changes
Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and ClassificationTypeNames

Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also server.CommandTypes

Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and HighlightSpanKind

@mhegazy

Mohamed Hegazy (mhegazy) commented May 22, 2017

Copy link
Copy Markdown
Contributor

Also please add a note for this in https://github.com/Microsoft/TypeScript/wiki/API-Breaking-Changes#typescript-24

@mhegazyMohamed Hegazy (mhegazy) added Breaking Change Would introduce errors in existing code Domain: API Relates to the public API for TypeScript labels May 22, 2017
@ghost

Deleted user (ghost) commented May 22, 2017

Copy link
Copy Markdown
Author

Mohamed Hegazy (@mhegazy) Is there any reason to avoid a non-const enum in protocol.ts? We iterate over for (const name in CommandNames) in a unit test (and now CommandNames = CommandTypes).

mhegazy
Mohamed Hegazy (mhegazy) reviewed May 22, 2017
View reviewed changes
Comment threadsrc/server/protocol.ts Outdated
namespace ts.server.protocol {
export namespace CommandTypes {
export type Brace = "brace";
export enum CommandTypes {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should be const enum, since it does not exist at time of building the client against the protocol file.

@ghostDeleted user (ghost)May 22, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We need to iterate over it though -- I guess I could define a CommandTypes[] elsewhere but we'd have to remember to update both places every time a new command is added.

mhegazy
Mohamed Hegazy (mhegazy) suggested changes May 22, 2017
View reviewed changes
Comment threadsrc/server/protocol.ts
export namespace NewLineKind {
export type Crlf = "Crlf";
export type Lf = "Lf";
export const enum NewLineKind {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

missing the initialization here.

@mhegazy

Mohamed Hegazy (mhegazy) commented May 22, 2017

Copy link
Copy Markdown
Contributor

Mohamed Hegazy (@mhegazy) Is there any reason to avoid a non-const enum in protocol.ts? We iterate over for (const name in CommandNames) in a unit test (and now CommandNames = CommandTypes).

yes. protocol.ts compiles down to protocol.d.ts which is then used by the clients to compile against.

so either we make them const in protocol.ts or do that while we put together protocol.d.ts in buildProtocol.js.

@ghost
ghost merged commit 73ee2fe into masterMay 23, 2017
@ghost
ghost deleted the kind branch May 23, 2017 14:19
@microsoftMicrosoft (microsoft) locked and limited conversation to collaborators Jun 14, 2018
This pull request was closed.
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Breaking ChangeWould introduce errors in existing codeDomain: APIRelates to the public API for TypeScript

Projects

None yet

Milestone

TypeScript 2.4

Development

Successfully merging this pull request may close these issues.

2 participants

@mhegazy@msftclas
, '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

Make ScriptElementKind and HighlightSpanKind string enums - #15966

Merged
6 commits merged into
masterfrom
kind
May 23, 2017
Merged

Make ScriptElementKind and HighlightSpanKind string enums#15966
6 commits merged into
masterfrom
kind

Conversation

@ghost

@ghostDeleted user (ghost) commented May 20, 2017

Copy link
Copy Markdown

Note: is technically a change to our public API as many strings are now ScriptElementKinds. And anyone using the API must now have ts@next or they will get compile errors on the string enums -- but presumably people compiling against the ts@next API are compiling using ts@next too.
Required running gulp LKG to get #15486 in.

mhegazy
Mohamed Hegazy (mhegazy) reviewed May 22, 2017
View reviewed changes
Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should also change ScriptElementKindModifier to be an enum.

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and ClassificationTypeNames

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also server.CommandTypes

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and HighlightSpanKind

mhegazy
Mohamed Hegazy (mhegazy) suggested changes May 22, 2017
View reviewed changes
Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and ClassificationTypeNames

Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also server.CommandTypes

Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and HighlightSpanKind

@mhegazy

Mohamed Hegazy (mhegazy) commented May 22, 2017

Copy link
Copy Markdown
Contributor

Also please add a note for this in https://github.com/Microsoft/TypeScript/wiki/API-Breaking-Changes#typescript-24

@mhegazyMohamed Hegazy (mhegazy) added Breaking Change Would introduce errors in existing code Domain: API Relates to the public API for TypeScript labels May 22, 2017
@ghost

Deleted user (ghost) commented May 22, 2017

Copy link
Copy Markdown
Author

Mohamed Hegazy (@mhegazy) Is there any reason to avoid a non-const enum in protocol.ts? We iterate over for (const name in CommandNames) in a unit test (and now CommandNames = CommandTypes).

mhegazy
Mohamed Hegazy (mhegazy) reviewed May 22, 2017
View reviewed changes
Comment threadsrc/server/protocol.ts Outdated
namespace ts.server.protocol {
export namespace CommandTypes {
export type Brace = "brace";
export enum CommandTypes {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should be const enum, since it does not exist at time of building the client against the protocol file.

@ghostDeleted user (ghost)May 22, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We need to iterate over it though -- I guess I could define a CommandTypes[] elsewhere but we'd have to remember to update both places every time a new command is added.

mhegazy
Mohamed Hegazy (mhegazy) suggested changes May 22, 2017
View reviewed changes
Comment threadsrc/server/protocol.ts
export namespace NewLineKind {
export type Crlf = "Crlf";
export type Lf = "Lf";
export const enum NewLineKind {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

missing the initialization here.

@mhegazy

Mohamed Hegazy (mhegazy) commented May 22, 2017

Copy link
Copy Markdown
Contributor

Mohamed Hegazy (@mhegazy) Is there any reason to avoid a non-const enum in protocol.ts? We iterate over for (const name in CommandNames) in a unit test (and now CommandNames = CommandTypes).

yes. protocol.ts compiles down to protocol.d.ts which is then used by the clients to compile against.

so either we make them const in protocol.ts or do that while we put together protocol.d.ts in buildProtocol.js.

@ghost
ghost merged commit 73ee2fe into masterMay 23, 2017
@ghost
ghost deleted the kind branch May 23, 2017 14:19
@microsoftMicrosoft (microsoft) locked and limited conversation to collaborators Jun 14, 2018
This pull request was closed.
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Breaking ChangeWould introduce errors in existing codeDomain: APIRelates to the public API for TypeScript

Projects

None yet

Milestone

TypeScript 2.4

Development

Successfully merging this pull request may close these issues.

2 participants

@mhegazy@msftclas
, '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 \u003e 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

Make ScriptElementKind and HighlightSpanKind string enums - #15966

Merged
6 commits merged into
masterfrom
kind
May 23, 2017
Merged

Make ScriptElementKind and HighlightSpanKind string enums#15966
6 commits merged into
masterfrom
kind

Conversation

@ghost

@ghostDeleted user (ghost) commented May 20, 2017

Copy link
Copy Markdown

Note: is technically a change to our public API as many strings are now ScriptElementKinds. And anyone using the API must now have ts@next or they will get compile errors on the string enums -- but presumably people compiling against the ts@next API are compiling using ts@next too.
Required running gulp LKG to get #15486 in.

mhegazy
Mohamed Hegazy (mhegazy) reviewed May 22, 2017
View reviewed changes
Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should also change ScriptElementKindModifier to be an enum.

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and ClassificationTypeNames

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also server.CommandTypes

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and HighlightSpanKind

mhegazy
Mohamed Hegazy (mhegazy) suggested changes May 22, 2017
View reviewed changes
Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and ClassificationTypeNames

Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also server.CommandTypes

Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and HighlightSpanKind

@mhegazy

Mohamed Hegazy (mhegazy) commented May 22, 2017

Copy link
Copy Markdown
Contributor

Also please add a note for this in https://github.com/Microsoft/TypeScript/wiki/API-Breaking-Changes#typescript-24

@mhegazyMohamed Hegazy (mhegazy) added Breaking Change Would introduce errors in existing code Domain: API Relates to the public API for TypeScript labels May 22, 2017
@ghost

Deleted user (ghost) commented May 22, 2017

Copy link
Copy Markdown
Author

Mohamed Hegazy (@mhegazy) Is there any reason to avoid a non-const enum in protocol.ts? We iterate over for (const name in CommandNames) in a unit test (and now CommandNames = CommandTypes).

mhegazy
Mohamed Hegazy (mhegazy) reviewed May 22, 2017
View reviewed changes
Comment threadsrc/server/protocol.ts Outdated
namespace ts.server.protocol {
export namespace CommandTypes {
export type Brace = "brace";
export enum CommandTypes {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should be const enum, since it does not exist at time of building the client against the protocol file.

@ghostDeleted user (ghost)May 22, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We need to iterate over it though -- I guess I could define a CommandTypes[] elsewhere but we'd have to remember to update both places every time a new command is added.

mhegazy
Mohamed Hegazy (mhegazy) suggested changes May 22, 2017
View reviewed changes
Comment threadsrc/server/protocol.ts
export namespace NewLineKind {
export type Crlf = "Crlf";
export type Lf = "Lf";
export const enum NewLineKind {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

missing the initialization here.

@mhegazy

Mohamed Hegazy (mhegazy) commented May 22, 2017

Copy link
Copy Markdown
Contributor

Mohamed Hegazy (@mhegazy) Is there any reason to avoid a non-const enum in protocol.ts? We iterate over for (const name in CommandNames) in a unit test (and now CommandNames = CommandTypes).

yes. protocol.ts compiles down to protocol.d.ts which is then used by the clients to compile against.

so either we make them const in protocol.ts or do that while we put together protocol.d.ts in buildProtocol.js.

@ghost
ghost merged commit 73ee2fe into masterMay 23, 2017
@ghost
ghost deleted the kind branch May 23, 2017 14:19
@microsoftMicrosoft (microsoft) locked and limited conversation to collaborators Jun 14, 2018
This pull request was closed.
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Breaking ChangeWould introduce errors in existing codeDomain: APIRelates to the public API for TypeScript

Projects

None yet

Milestone

TypeScript 2.4

Development

Successfully merging this pull request may close these issues.

2 participants

@mhegazy@msftclas
, '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

Make ScriptElementKind and HighlightSpanKind string enums - #15966

Merged
6 commits merged into
masterfrom
kind
May 23, 2017
Merged

Make ScriptElementKind and HighlightSpanKind string enums#15966
6 commits merged into
masterfrom
kind

Conversation

@ghost

@ghostDeleted user (ghost) commented May 20, 2017

Copy link
Copy Markdown

Note: is technically a change to our public API as many strings are now ScriptElementKinds. And anyone using the API must now have ts@next or they will get compile errors on the string enums -- but presumably people compiling against the ts@next API are compiling using ts@next too.
Required running gulp LKG to get #15486 in.

mhegazy
Mohamed Hegazy (mhegazy) reviewed May 22, 2017
View reviewed changes
Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should also change ScriptElementKindModifier to be an enum.

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and ClassificationTypeNames

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also server.CommandTypes

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and HighlightSpanKind

mhegazy
Mohamed Hegazy (mhegazy) suggested changes May 22, 2017
View reviewed changes
Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and ClassificationTypeNames

Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also server.CommandTypes

Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and HighlightSpanKind

@mhegazy

Mohamed Hegazy (mhegazy) commented May 22, 2017

Copy link
Copy Markdown
Contributor

Also please add a note for this in https://github.com/Microsoft/TypeScript/wiki/API-Breaking-Changes#typescript-24

@mhegazyMohamed Hegazy (mhegazy) added Breaking Change Would introduce errors in existing code Domain: API Relates to the public API for TypeScript labels May 22, 2017
@ghost

Deleted user (ghost) commented May 22, 2017

Copy link
Copy Markdown
Author

Mohamed Hegazy (@mhegazy) Is there any reason to avoid a non-const enum in protocol.ts? We iterate over for (const name in CommandNames) in a unit test (and now CommandNames = CommandTypes).

mhegazy
Mohamed Hegazy (mhegazy) reviewed May 22, 2017
View reviewed changes
Comment threadsrc/server/protocol.ts Outdated
namespace ts.server.protocol {
export namespace CommandTypes {
export type Brace = "brace";
export enum CommandTypes {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should be const enum, since it does not exist at time of building the client against the protocol file.

@ghostDeleted user (ghost)May 22, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We need to iterate over it though -- I guess I could define a CommandTypes[] elsewhere but we'd have to remember to update both places every time a new command is added.

mhegazy
Mohamed Hegazy (mhegazy) suggested changes May 22, 2017
View reviewed changes
Comment threadsrc/server/protocol.ts
export namespace NewLineKind {
export type Crlf = "Crlf";
export type Lf = "Lf";
export const enum NewLineKind {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

missing the initialization here.

@mhegazy

Mohamed Hegazy (mhegazy) commented May 22, 2017

Copy link
Copy Markdown
Contributor

Mohamed Hegazy (@mhegazy) Is there any reason to avoid a non-const enum in protocol.ts? We iterate over for (const name in CommandNames) in a unit test (and now CommandNames = CommandTypes).

yes. protocol.ts compiles down to protocol.d.ts which is then used by the clients to compile against.

so either we make them const in protocol.ts or do that while we put together protocol.d.ts in buildProtocol.js.

@ghost
ghost merged commit 73ee2fe into masterMay 23, 2017
@ghost
ghost deleted the kind branch May 23, 2017 14:19
@microsoftMicrosoft (microsoft) locked and limited conversation to collaborators Jun 14, 2018
This pull request was closed.
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Breaking ChangeWould introduce errors in existing codeDomain: APIRelates to the public API for TypeScript

Projects

None yet

Milestone

TypeScript 2.4

Development

Successfully merging this pull request may close these issues.

2 participants

@mhegazy@msftclas
, '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

Make ScriptElementKind and HighlightSpanKind string enums - #15966

Merged
6 commits merged into
masterfrom
kind
May 23, 2017
Merged

Make ScriptElementKind and HighlightSpanKind string enums#15966
6 commits merged into
masterfrom
kind

Conversation

@ghost

@ghostDeleted user (ghost) commented May 20, 2017

Copy link
Copy Markdown

Note: is technically a change to our public API as many strings are now ScriptElementKinds. And anyone using the API must now have ts@next or they will get compile errors on the string enums -- but presumably people compiling against the ts@next API are compiling using ts@next too.
Required running gulp LKG to get #15486 in.

mhegazy
Mohamed Hegazy (mhegazy) reviewed May 22, 2017
View reviewed changes
Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should also change ScriptElementKindModifier to be an enum.

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and ClassificationTypeNames

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also server.CommandTypes

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and HighlightSpanKind

mhegazy
Mohamed Hegazy (mhegazy) suggested changes May 22, 2017
View reviewed changes
Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and ClassificationTypeNames

Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also server.CommandTypes

Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and HighlightSpanKind

@mhegazy

Mohamed Hegazy (mhegazy) commented May 22, 2017

Copy link
Copy Markdown
Contributor

Also please add a note for this in https://github.com/Microsoft/TypeScript/wiki/API-Breaking-Changes#typescript-24

@mhegazyMohamed Hegazy (mhegazy) added Breaking Change Would introduce errors in existing code Domain: API Relates to the public API for TypeScript labels May 22, 2017
@ghost

Deleted user (ghost) commented May 22, 2017

Copy link
Copy Markdown
Author

Mohamed Hegazy (@mhegazy) Is there any reason to avoid a non-const enum in protocol.ts? We iterate over for (const name in CommandNames) in a unit test (and now CommandNames = CommandTypes).

mhegazy
Mohamed Hegazy (mhegazy) reviewed May 22, 2017
View reviewed changes
Comment threadsrc/server/protocol.ts Outdated
namespace ts.server.protocol {
export namespace CommandTypes {
export type Brace = "brace";
export enum CommandTypes {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should be const enum, since it does not exist at time of building the client against the protocol file.

@ghostDeleted user (ghost)May 22, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We need to iterate over it though -- I guess I could define a CommandTypes[] elsewhere but we'd have to remember to update both places every time a new command is added.

mhegazy
Mohamed Hegazy (mhegazy) suggested changes May 22, 2017
View reviewed changes
Comment threadsrc/server/protocol.ts
export namespace NewLineKind {
export type Crlf = "Crlf";
export type Lf = "Lf";
export const enum NewLineKind {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

missing the initialization here.

@mhegazy

Mohamed Hegazy (mhegazy) commented May 22, 2017

Copy link
Copy Markdown
Contributor

Mohamed Hegazy (@mhegazy) Is there any reason to avoid a non-const enum in protocol.ts? We iterate over for (const name in CommandNames) in a unit test (and now CommandNames = CommandTypes).

yes. protocol.ts compiles down to protocol.d.ts which is then used by the clients to compile against.

so either we make them const in protocol.ts or do that while we put together protocol.d.ts in buildProtocol.js.

@ghost
ghost merged commit 73ee2fe into masterMay 23, 2017
@ghost
ghost deleted the kind branch May 23, 2017 14:19
@microsoftMicrosoft (microsoft) locked and limited conversation to collaborators Jun 14, 2018
This pull request was closed.
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Breaking ChangeWould introduce errors in existing codeDomain: APIRelates to the public API for TypeScript

Projects

None yet

Milestone

TypeScript 2.4

Development

Successfully merging this pull request may close these issues.

2 participants

@mhegazy@msftclas
, '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

Make ScriptElementKind and HighlightSpanKind string enums - #15966

Merged
6 commits merged into
masterfrom
kind
May 23, 2017
Merged

Make ScriptElementKind and HighlightSpanKind string enums#15966
6 commits merged into
masterfrom
kind

Conversation

@ghost

@ghostDeleted user (ghost) commented May 20, 2017

Copy link
Copy Markdown

Note: is technically a change to our public API as many strings are now ScriptElementKinds. And anyone using the API must now have ts@next or they will get compile errors on the string enums -- but presumably people compiling against the ts@next API are compiling using ts@next too.
Required running gulp LKG to get #15486 in.

mhegazy
Mohamed Hegazy (mhegazy) reviewed May 22, 2017
View reviewed changes
Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should also change ScriptElementKindModifier to be an enum.

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and ClassificationTypeNames

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also server.CommandTypes

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and HighlightSpanKind

mhegazy
Mohamed Hegazy (mhegazy) suggested changes May 22, 2017
View reviewed changes
Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and ClassificationTypeNames

Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also server.CommandTypes

Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and HighlightSpanKind

@mhegazy

Mohamed Hegazy (mhegazy) commented May 22, 2017

Copy link
Copy Markdown
Contributor

Also please add a note for this in https://github.com/Microsoft/TypeScript/wiki/API-Breaking-Changes#typescript-24

@mhegazyMohamed Hegazy (mhegazy) added Breaking Change Would introduce errors in existing code Domain: API Relates to the public API for TypeScript labels May 22, 2017
@ghost

Deleted user (ghost) commented May 22, 2017

Copy link
Copy Markdown
Author

Mohamed Hegazy (@mhegazy) Is there any reason to avoid a non-const enum in protocol.ts? We iterate over for (const name in CommandNames) in a unit test (and now CommandNames = CommandTypes).

mhegazy
Mohamed Hegazy (mhegazy) reviewed May 22, 2017
View reviewed changes
Comment threadsrc/server/protocol.ts Outdated
namespace ts.server.protocol {
export namespace CommandTypes {
export type Brace = "brace";
export enum CommandTypes {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should be const enum, since it does not exist at time of building the client against the protocol file.

@ghostDeleted user (ghost)May 22, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We need to iterate over it though -- I guess I could define a CommandTypes[] elsewhere but we'd have to remember to update both places every time a new command is added.

mhegazy
Mohamed Hegazy (mhegazy) suggested changes May 22, 2017
View reviewed changes
Comment threadsrc/server/protocol.ts
export namespace NewLineKind {
export type Crlf = "Crlf";
export type Lf = "Lf";
export const enum NewLineKind {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

missing the initialization here.

@mhegazy

Mohamed Hegazy (mhegazy) commented May 22, 2017

Copy link
Copy Markdown
Contributor

Mohamed Hegazy (@mhegazy) Is there any reason to avoid a non-const enum in protocol.ts? We iterate over for (const name in CommandNames) in a unit test (and now CommandNames = CommandTypes).

yes. protocol.ts compiles down to protocol.d.ts which is then used by the clients to compile against.

so either we make them const in protocol.ts or do that while we put together protocol.d.ts in buildProtocol.js.

@ghost
ghost merged commit 73ee2fe into masterMay 23, 2017
@ghost
ghost deleted the kind branch May 23, 2017 14:19
@microsoftMicrosoft (microsoft) locked and limited conversation to collaborators Jun 14, 2018
This pull request was closed.
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Breaking ChangeWould introduce errors in existing codeDomain: APIRelates to the public API for TypeScript

Projects

None yet

Milestone

TypeScript 2.4

Development

Successfully merging this pull request may close these issues.

2 participants

@mhegazy@msftclas
, '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

Make ScriptElementKind and HighlightSpanKind string enums - #15966

Merged
6 commits merged into
masterfrom
kind
May 23, 2017
Merged

Make ScriptElementKind and HighlightSpanKind string enums#15966
6 commits merged into
masterfrom
kind

Conversation

@ghost

@ghostDeleted user (ghost) commented May 20, 2017

Copy link
Copy Markdown

Note: is technically a change to our public API as many strings are now ScriptElementKinds. And anyone using the API must now have ts@next or they will get compile errors on the string enums -- but presumably people compiling against the ts@next API are compiling using ts@next too.
Required running gulp LKG to get #15486 in.

mhegazy
Mohamed Hegazy (mhegazy) reviewed May 22, 2017
View reviewed changes
Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should also change ScriptElementKindModifier to be an enum.

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and ClassificationTypeNames

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also server.CommandTypes

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and HighlightSpanKind

mhegazy
Mohamed Hegazy (mhegazy) suggested changes May 22, 2017
View reviewed changes
Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and ClassificationTypeNames

Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also server.CommandTypes

Comment threadsrc/services/types.ts Outdated
jsxAttribute = "JSX attribute",
}

export namespace ScriptElementKindModifier {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and HighlightSpanKind

@mhegazy

Mohamed Hegazy (mhegazy) commented May 22, 2017

Copy link
Copy Markdown
Contributor

Also please add a note for this in https://github.com/Microsoft/TypeScript/wiki/API-Breaking-Changes#typescript-24

@mhegazyMohamed Hegazy (mhegazy) added Breaking Change Would introduce errors in existing code Domain: API Relates to the public API for TypeScript labels May 22, 2017
@ghost

Deleted user (ghost) commented May 22, 2017

Copy link
Copy Markdown
Author

Mohamed Hegazy (@mhegazy) Is there any reason to avoid a non-const enum in protocol.ts? We iterate over for (const name in CommandNames) in a unit test (and now CommandNames = CommandTypes).

mhegazy
Mohamed Hegazy (mhegazy) reviewed May 22, 2017
View reviewed changes
Comment threadsrc/server/protocol.ts Outdated
namespace ts.server.protocol {
export namespace CommandTypes {
export type Brace = "brace";
export enum CommandTypes {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should be const enum, since it does not exist at time of building the client against the protocol file.

@ghostDeleted user (ghost)May 22, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We need to iterate over it though -- I guess I could define a CommandTypes[] elsewhere but we'd have to remember to update both places every time a new command is added.

mhegazy
Mohamed Hegazy (mhegazy) suggested changes May 22, 2017
View reviewed changes
Comment threadsrc/server/protocol.ts
export namespace NewLineKind {
export type Crlf = "Crlf";
export type Lf = "Lf";
export const enum NewLineKind {

@mhegazyMohamed Hegazy (mhegazy)May 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

missing the initialization here.

@mhegazy

Mohamed Hegazy (mhegazy) commented May 22, 2017

Copy link
Copy Markdown
Contributor

Mohamed Hegazy (@mhegazy) Is there any reason to avoid a non-const enum in protocol.ts? We iterate over for (const name in CommandNames) in a unit test (and now CommandNames = CommandTypes).

yes. protocol.ts compiles down to protocol.d.ts which is then used by the clients to compile against.

so either we make them const in protocol.ts or do that while we put together protocol.d.ts in buildProtocol.js.

@ghost
ghost merged commit 73ee2fe into masterMay 23, 2017
@ghost
ghost deleted the kind branch May 23, 2017 14:19
@microsoftMicrosoft (microsoft) locked and limited conversation to collaborators Jun 14, 2018
This pull request was closed.
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Breaking ChangeWould introduce errors in existing codeDomain: APIRelates to the public API for TypeScript

Projects

None yet

Milestone

TypeScript 2.4

Development

Successfully merging this pull request may close these issues.

2 participants

@mhegazy@msftclas