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 back typescript version number and add Deno.version object. #1788

Merged
merged 7 commits into from Feb 18, 2019

Conversation

3 participants
@kt3k
Copy link
Contributor

kt3k commented Feb 16, 2019

This is a continuation of #1676. This adds back the printing of typescript version number.
This also addresses the comment:

t would be good to have access to the versions in the runtime environment - maybe that can be done as part of this work... deno.version.v8 for example would be nice to have.

This PR uses rollup-plugin-replace for embedding typescript version in the bundle.


depends on denoland/deno_third_party#33

@kt3k kt3k force-pushed the kt3k:feature/ts-version branch from 92bf38d to 93a0f8b Feb 16, 2019

@kitsonk
Copy link
Contributor

kitsonk left a comment

Couple comments.

Also the diff is pulling in sourcemap-codec into third_party/node_modules and I am not sure why. Seems like something we don't want/need in the project.

@@ -0,0 +1,15 @@
// Copyright 2018-2019 the Deno authors. All rights reserved. MIT license.
// tslint:disable-next-line:no-reference
/// <reference path="./const.d.ts" />

This comment has been minimized.

@kitsonk

kitsonk Feb 16, 2019

Contributor

Instead of this, I would just add it to the globals in globals.ts:

deno/js/globals.ts

Lines 35 to 38 in 4dc4329

declare global {
const console: consoleTypes.Console;
const setTimeout: typeof timers.setTimeout;
}

Then you won't need an additional type declaration file.

This comment has been minimized.

@kt3k

kt3k Feb 16, 2019

Author Contributor

The situation seems a little tricky here. rollup-plugin-replace replaces the occurence even in type declaration. So const TS_VERSION: string becomes const "3.2.1": string; before passed to typescript.

I found that rollup-plugin-replace doesn't do any parsing of the source code. So we can do something like: { version: "TS_VERSION" }. This seems better because it doesn't require any type declaration. So I swtich to this pattern.

js/main.ts Outdated
@@ -24,12 +25,14 @@ export default function denoMain() {
libdeno.builtinModules["deno"] = deno;
Object.freeze(libdeno.builtinModules);

version.deno = startResMsg.denoVersion();

This comment has been minimized.

@kitsonk

kitsonk Feb 16, 2019

Contributor

I would consider adding a set function that is exported from version.ts and marked @internal to do the setting, like we have with setEnv() now. This will make it more maintainable in the long term and keep logic out of here. And setVersions() should freeze the version object too.

@kt3k kt3k force-pushed the kt3k:feature/ts-version branch from eb0cb79 to 269aba6 Feb 16, 2019

@kt3k

This comment has been minimized.

Copy link
Contributor Author

kt3k commented Feb 16, 2019

@kitsonk

the diff is pulling in sourcemap-codec into third_party/node_modules and I am not sure why. Seems like something we don't want/need in the project.

rollup-plugin-replace@2.1.0 depends on magic-string@^0.25.1.
magic-string seems depending on sourcemap-codec since v0.23.0 Rich-Harris/magic-string#133

If we pinned the version of rollup-plugin-replace to v2.0.0 which depends magic-string@^0.22.4, we would remove sourcemap-codec dependecy. But I'm not sure that is the desired solution.

refactor: use TS_VERSION in string literal
This doesn't need any type declartion

@kt3k kt3k force-pushed the kt3k:feature/ts-version branch from 269aba6 to 849b184 Feb 16, 2019

kt3k added some commits Feb 16, 2019

@kitsonk
Copy link
Contributor

kitsonk left a comment

One final comment.

I understand the sourcemap-codec as well now, so am ok with that.

};

/**
* Sets the deno and v8 versions and freezes the version object.

This comment has been minimized.

@kitsonk

kitsonk Feb 16, 2019

Contributor

I would set this to @internal so it doesn't get leaked to the runtime type library.

@kitsonk
Copy link
Contributor

kitsonk left a comment

LGTM

@ry

ry approved these changes Feb 18, 2019

Copy link
Collaborator

ry left a comment

LGTM - this is great to have back - and also Deno.version - thank you!

@ry ry merged commit 55edc06 into denoland:master Feb 18, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.