Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move to TypeScript 2.6.1 #3559

Merged
merged 24 commits into from Dec 11, 2017
Merged

Move to TypeScript 2.6.1 #3559

merged 24 commits into from Dec 11, 2017

Conversation

riknoll
Copy link
Member

@riknoll riknoll commented Dec 4, 2017

This moves us from TypeScript 1.8.7 to Typescript 2.6.1 (both for our compiler and our code). Benefits include:

This is a very dangerous change and should not be merged before any big releases. It is also a breaking change. For example, this code is no longer legal:

if (2 === 3) {
 // do something
}

TypeScript 2.0+ considers that comparison to be between the types 2 and 3 and not two numbers, so this comes up as an error. I'm sure there is other stuff I'm not thinking of.

Things still to be done:

  • Not sure how much needs to be done with the Monaco language service
  • Update our React version (we were waiting on the new types)
  • Use Async await all over the place

Testing

  • basic test run against Adafruit
  • basic test run against Minecraft
  • basic test run against Chibitronics

@@ -108,8 +108,19 @@
"semantic-ui-less": "^2.2.11",
Copy link
Member

Choose a reason for hiding this comment

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

dump the minor version as well

@@ -302,7 +302,7 @@ namespace ts.pxtc.decompiler {

export function decompileToBlocks(blocksInfo: pxtc.BlocksInfo, file: ts.SourceFile, options: DecompileBlocksOptions, renameMap?: RenameMap): pxtc.CompileResult {
let emittedBlocks = 0;
let stmts: ts.Statement[] = file.statements;
let stmts: NodeArray<ts.Statement> = file.statements;
Copy link
Member

Choose a reason for hiding this comment

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

is this a new type?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, but the type definition in the TS compiler changed so it can no longer be assigned to a normal array. Pretty sure the reason is that the indices are readonly in the NodeArray (which is a TS 2.0 feature).

@@ -566,7 +564,7 @@ namespace ts.pxtc.service {
}
}

const operations: pxt.Map<(v: OpArg) => any> = {
const operations: pxt.Map<(v: any) => any> = {
Copy link
Member

Choose a reason for hiding this comment

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

any?

@@ -249,12 +248,12 @@ namespace pxt.docs {

if (currentTocEntry) {
if (currentTocEntry.prevPath) {
params["prev"] = `<a href="${currentTocEntry.prevPath}" class="navigation navigation-prev " title="${'Previous page: {0}', currentTocEntry.prevName}">
params["prev"] = `<a href="${currentTocEntry.prevPath}" class="navigation navigation-prev " title="${currentTocEntry.prevName}">
Copy link
Member

Choose a reason for hiding this comment

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

why this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Look at the comma; the left side isn't doing anything so the TS compiler was throwing an error. I'm not sure what the desired behavior is, but my change shouldn't affect the current behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Oh... it is missing lf("..."). Let's keep this change. simpler.

@@ -513,16 +513,6 @@ namespace pxsim {
return new RefBuffer(buf.data.slice(offset, offset + length));
}

export function toHex(buf: RefBuffer): string {
Copy link
Member

Choose a reason for hiding this comment

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

gone?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not used anywhere

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, it's a sim-shim. I'll bring it back

Copy link
Member

@pelikhan pelikhan left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -73,15 +73,15 @@ namespace pxsim.svg {


export function fill(el: SVGElement, c: string) {
(<SVGStylable><any>el).style.fill = c;
Copy link
Member

Choose a reason for hiding this comment

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

No more SVGStyleable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I couldn't find it in lib.d.ts. Either it's gone or the name changed.

Copy link
Member

Choose a reason for hiding this comment

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

sniff :(

@@ -309,7 +325,7 @@ namespace ts.pxtc {
while (true) {
node = node.parent
if (!node)
userError(9229, lf("cannot determine parent of {0}", stringKind(node0)))
userError(9229, lf("cannot determine parent of {0}", stringKind(node0)))
Copy link
Member

Choose a reason for hiding this comment

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

missing tab?

@@ -557,6 +599,39 @@ namespace ts.pxtc {
return checkType(r)
}

function checkUnionOfLiterals(t: Type): HasLiteralType {
Copy link
Member

Choose a reason for hiding this comment

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

coool! do we have tests on those?

msg = lf("{0} not supported", ts.SyntaxKind[n.kind])
}

let t = (n as any) as any[];
Copy link
Member

Choose a reason for hiding this comment

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

left over logging

@@ -3310,13 +3404,13 @@ ${lbl}: .short 0xffff
return r;
let tp = typeOf(e)

if (target.floatingPoint && (tp.flags & (TypeFlags.NumberLike | TypeFlags.Boolean)))
if (target.floatingPoint && (tp.flags & (TypeFlags.NumberLike | TypeFlags.Boolean | TypeFlags.BooleanLiteral)))
Copy link
Member

Choose a reason for hiding this comment

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

at this point, we should really clean out the floating point stuff

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it needs a general cleanup but I'm convinced we should leave support for integer compilation for tiny targets (AVR, maybe some tiny ARMS)

Copy link
Member

@mmoskal mmoskal left a comment

Choose a reason for hiding this comment

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

Wow! That's a lot of changes - reminds me updates to TD back when TS was changing even faster ;))

Looks great!

@@ -2300,14 +2302,14 @@ class Host
check(p)

if (U.endsWith(filename, ".uf2"))
fs.writeFileSync(p, contents, "base64")
fs.writeFileSync(p, contents, { encoding: "base64" })
Copy link
Member

Choose a reason for hiding this comment

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

Node.js doesn't seem to deprecate string as encoding usage. Is the @types missing it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, you're right. Not sure why it was removed from the @types

@riknoll riknoll merged commit 749c2c4 into master Dec 11, 2017
@riknoll riknoll deleted the new_ts branch February 17, 2018 01:25
@lock
Copy link

lock bot commented Sep 26, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants