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

Add --types #952

Closed
wants to merge 3 commits into from
Closed

Add --types #952

wants to merge 3 commits into from

Conversation

ry
Copy link
Member

@ry ry commented Oct 10, 2018

Continuation of #729 - this patch has my approval.

Due to the size and complexity of this, I want to do another round of reviews with @piscisaureus

The output of ./out/debug/deno --types looks like this: https://gist.github.com/ry/906d9aa89e0fa6a4b17a21276d5158c5

Fixes #632

@ry ry requested a review from piscisaureus October 10, 2018 13:33
@ry ry mentioned this pull request Oct 10, 2018
@ry
Copy link
Member Author

ry commented Oct 10, 2018

@kitsonk @dsherret Any idea about this Windows-only error?

c:/Python27-x64/python.exe ../../tools/run_node.py ../../tools/ts_library_builder/main --basePath ../.. --buildPath . --outFile gen/lib/lib.deno_runtime.d.ts --silent
c:\deno\third_party\node_modules\ts-simple-ast\dist\errors\classes\BaseError.js:8
        var _this = _super.call(this, message) || this;
                           ^
Error: Error replacing tree! Perhaps a syntax error was inserted (Current: JSDocComment -- New: FunctionKeyword).
-- Details --
Path: c:/deno/js/util.ts
Text: ".../types\";\n\nlet logDebug = false;\n\nfunction setLogDebug(debug: boolean): void {\n  logDebug = debug;\n}\n\nfunction log(...args: any[]): void {\n  if (logDebug) {\n    console.log(\"DEBUG JS -\", ...args);\n  }\n}\n\n// @internal\nexport function asser..."
    at InvalidOperationError.BaseError [as constructor] (c:\deno\third_party\node_modules\ts-simple-ast\dist\errors\classes\BaseError.js:8:28)
    at new InvalidOperationError (c:\deno\third_party\node_modules\ts-simple-ast\dist\errors\classes\InvalidOperationError.js:9:23)
    at Object.doManipulation (c:\deno\third_party\node_modules\ts-simple-ast\dist\manipulation\manipulations\doManipulation.js:12:15)
    at Object.removeChildren (c:\deno\third_party\node_modules\ts-simple-ast\dist\manipulation\manipulations\removal.js:13:22)
    at FunctionDeclaration.ModifierableNode.class_1.removeModifier (c:\deno\third_party\node_modules\ts-simple-ast\dist\compiler\ast\base\ModifierableNode.js:136:28)
    at FunctionDeclaration.ModifierableNode.class_1.toggleModifier (c:\deno\third_party\node_modules\ts-simple-ast\dist\compiler\ast\base\ModifierableNode.js:51:22)
    at FunctionDeclaration.ExportableNode.class_1.setIsExported (c:\deno\third_party\node_modules\ts-simple-ast\dist\compiler\ast\base\ExportableNode.js:97:18)
    at currentSourceFile.forEachChild.node (c:\deno\tools\ts_library_builder\ast_util.ts:124:16)
    at SourceFile.Node.forEachChild (c:\deno\third_party\node_modules\ts-simple-ast\dist\compiler\ast\common\Node.js:468:21

@dsherret
Copy link
Member

@ry @kitsonk that's strange it only happens on windows. Luckily I am able to reproduce in a simple example. I'll do a fix... sorry about that!

@ry
Copy link
Member Author

ry commented Oct 10, 2018

@dsherret Thanks for looking into it!

How tied are we going to be to TS 3.0.3 ? Are you planning on upgrading to 3.1 soon? Is that difficult?

@dsherret
Copy link
Member

@ry update to 16.0.4 and it should be fixed. Basically, it was removing the jsdoc when removing the declare modifier (the node position in the ast ignores jsdocs and comments).

The work for being untied to 3.0.3 will be in version 17 and the work for that is basically done. I have tests that run through every supported compiler version to ensure there are no problems. I'm just adding a few small things to v17, but I should have a release ready for the weekend. I'll do a PR here when that's done.

Adds //tools/ts_library_builder for building a unified .d.ts file for
the deno runtime called lib.deno_runtime.d.ts

This serves as a low-level documentation system for Deno.
@ry
Copy link
Member Author

ry commented Oct 10, 2018

@dsherret Thanks for looking into it !

@ry
Copy link
Member Author

ry commented Oct 10, 2018

@dsherret That definitely fixed one error - but it seems others have popped up. Any idea why this is open happening on Windows?

@dsherret
Copy link
Member

@ry ok, that's very strange. I'll have to take a look tonight (if you get a chance, could you post the error you're getting here? It may take me a little time to setup the dependencies to build deno on windows and I could look into the error while everything is installing).

No, I'm not sure. Any errors like that previous one should happen on both linux and windows. I'll look into it though. The compiler api shouldn't be parsing files differently based on the platform so perhaps there's some other external reason.

Sorry once again! I have lots of tests and any errors we encounter here will be added to the test suite to ensure they don't happen in the future.

@ry
Copy link
Member Author

ry commented Oct 10, 2018

@dsherret Thanks I appreciate it!

Looking at the error now - I wonder if this is even a problem in ts-simple-ast or if it's a bug in @kitsonk's code?

https://gist.github.com/ry/94652eb7cea628792bce4960c4ae96db

@dsherret
Copy link
Member

@ry ok, I think it's because of these lines:

const jsPath = join(basePath, "js");
  const inputProjectFiles = inputProject
    .getSourceFiles()
    .map(sourceFile => sourceFile.getFilePath())
    .filter(filePath => !filePath.startsWith(jsPath));
  loadFiles(declarationProject, inputProjectFiles);

The TypeScript compiler uses forwards slashes for everything regardless of platform (see here and the normalizeSlashes function below), so most likely jsPath has backslashes on windows and so the startsWith never matches.

VariableStatement,
VariableDeclarationKind
} from "ts-simple-ast";
import * as ts from "typescript";
Copy link
Member

Choose a reason for hiding this comment

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

@kitsonk sorry, didn't take a look at this until now. I recommend using the ts named export from "ts-simple-ast" instead. That way you ensure the code always uses the typescript dependency that ts-simple-ast is relying on (just in case they ever happen to be different).

@ry
Copy link
Member Author

ry commented Oct 10, 2018

@dsherret Thanks for debugging - that does seem to be the problem.

@kitsonk We could really use unit tests here... I've added some scaffold for them.

@kitsonk
Copy link
Contributor

kitsonk commented Oct 10, 2018

@ry I will work on unit tests today. 👍

@ry
Copy link
Member Author

ry commented Oct 10, 2018

Awesome - thanks! I will close this PR then since it's not ready to land.

@ry ry closed this Oct 10, 2018
const inputProjectFiles = inputProject
.getSourceFiles()
.map(sourceFile => sourceFile.getFilePath())
.filter(filePath => !filePath.startsWith(jsPath));
Copy link
Member

Choose a reason for hiding this comment

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

@kitsonk Was just thinking about this. I think this would work?

const jsDir = inputProject.getDirectoryOrThrow(join(basePath, "js"));
const inputProjectFiles = jsDir.getDescendantSourceFiles();

Copy link
Member

Choose a reason for hiding this comment

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

Oh, whoops! It's the opposite of that. Try this:

const jsDir = inputProject.getDirectoryOrThrow(join(basePath, "js"));
const inputProjectFiles = inputProject
    .getSourceFiles()
    .filter(sourceFile => !jsDir.isAncestorOf(sourceFile));

Copy link
Contributor

Choose a reason for hiding this comment

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

What I did in the end for now was just create a normalizePath() that replicates the TypeScript functionality (before I really took a look at this comment)... It looks like it is finally passing CI.

@kitsonk kitsonk mentioned this pull request Oct 11, 2018
@ry ry mentioned this pull request Oct 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants