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

chore(build): Bump typescript to version 3.6.4 #27

Merged
merged 1 commit into from
Dec 11, 2019

Conversation

GordonSmith
Copy link
Contributor

Signed-off-by: Gordon Smith [email protected]

@GordonSmith
Copy link
Contributor Author

Note: You should avoid upgrading to TS compiler 3.7.x due to: microsoft/TypeScript#33939

@afshin
Copy link
Member

afshin commented Dec 10, 2019

cc: @jasongrout, who is usually interested in the compiler version

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@vidartf
Copy link
Member

vidartf commented Dec 11, 2019

Will this change cause builds by consumers to fail if they use an older version of TS?

@GordonSmith
Copy link
Contributor Author

@vidartf I don't believe so - unfortunately the same is not true of 3.7.x

Copy link
Member

@vidartf vidartf left a comment

Choose a reason for hiding this comment

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

Would you mind merging master into this (or rebasing) and ensuring that the polling package also gets the update?

@@ -494,6 +494,7 @@ namespace MessageLoop {
*/
const schedule = (() => {
let ok = typeof requestAnimationFrame === 'function';
// @ts-ignore setImmediate is valid for NodeJS
Copy link
Member

Choose a reason for hiding this comment

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

Could this be resolved by adding a dev dependency on @types/node?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so as its not a NodeJS API "thing" (its a global "window"/"this" thing).

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that is a different one to the "window.setImmediate" that lumino is using (the v8 JS runtime one)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(IOW timers.setImmediate vs GLOBAL.setImmediate)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

As the typing of schedule/unschedule is not exported, it should suffice to ensure that it compiles, so v12 seems reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no luck (after testing) and visually inspecting the NodeJS typings its declared in the "timers" module and not globally.

There is a @types/setimmediate which should work - but do you want to go down that road?

At some point you might want a "platform" test:

declare const process: any;

//  @ts-ignore
export const root: any = new Function("return this;")(); //  Prevent bundlers from messing with "this"

export const isBrowser: boolean = typeof window !== "undefined" && root === window;
export const isNode: boolean = typeof process !== "undefined" && process.versions != null && process.versions.node != null;
export const isTravis: boolean = isNode && process.env != null && process.env.TRAVIS != null;

See: https://github.com/hpcc-systems/Visualization/blob/master/packages/util/src/platform.ts

And will be able to add TypeScript guards for things like setImmediate.

Its still pretty tricky getting a single TypeScript package to "just work" for Node and Browser especially when you get into fetch polyfills and the like...

Copy link
Member

Choose a reason for hiding this comment

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

@GordonSmith Did you see my link to the global.d.ts file? Also, did you add the types dependency to tsconfig.json's types field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see that now - pushed new version with suggested change.

packages/datagrid/src/graphicscontext.ts Show resolved Hide resolved
@GordonSmith
Copy link
Contributor Author

I just did a diff of the generated files - the only potential issue I see is the use of the "correct" types in graphicscontext.d.ts (do older TS versions have access to the latest "lib.dom.d.ts")?

FWIW I have not used these types in my other projects (which have had no issues with older TSC versions).

@GordonSmith
Copy link
Contributor Author

I believe polling was already included?

@vidartf
Copy link
Member

vidartf commented Dec 11, 2019

I believe polling was already included?

I don't see the package.json in the diff? And a yarn why typescript off the branch still reported polling as being on ~3.0.3..

@vidartf
Copy link
Member

vidartf commented Dec 11, 2019

(I can push these changes if you are not at a computer)

@GordonSmith
Copy link
Contributor Author

I just rebased and see that now (I suspect I just visually missed it before as polling wasn't in my original branch).

I see that "notebooks" is on 3.7.2 should this also be 3.6.4?

@GordonSmith
Copy link
Contributor Author

What is notebooks and should it be in lumino?

@vidartf
Copy link
Member

vidartf commented Dec 11, 2019

What is notebooks and should it be in lumino?

Take it up with @ellisonbg #8 (it is just some maintainer code to discuss code in the datastore package, but I would argue it should be a gist instead).

@GordonSmith
Copy link
Contributor Author

GordonSmith commented Dec 11, 2019

If it (notebooks) doesn't make it into the npm tar file, then its really up to you folks (I can't say I even noticed it was there).
Edit: But rolling its TS version back to 3.0.3 might be a good sanity check?

@vidartf
Copy link
Member

vidartf commented Dec 11, 2019

If it (notebooks) doesn't make it into the npm tar file

it doesn't. (or rather, if it did, it would only make it into its own tar file, as each package is separated)

@GordonSmith GordonSmith force-pushed the TSC_3_6 branch 3 times, most recently from 8b1d732 to 634fece Compare December 11, 2019 16:09
@GordonSmith
Copy link
Contributor Author

Pushed changes for polling which will need a new review.

@GordonSmith
Copy link
Contributor Author

Pushed with @types/node included, the following will need re-review:

  • messaging
  • polling

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@blink1073 blink1073 merged commit cff0fbe into jupyterlab:master Dec 11, 2019
@GordonSmith GordonSmith deleted the TSC_3_6 branch December 11, 2019 16:41
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants