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

Use TypeScript 3.1 #5360

Merged
merged 11 commits into from Sep 28, 2018
Merged

Use TypeScript 3.1 #5360

merged 11 commits into from Sep 28, 2018

Conversation

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 22, 2018

Takes advantage of the new project references feature to simplify how we build our packages.

Everything Just Works ™️ in that each folder gets its own lib/ as if it were built directly, huzzah! However, because the output of metapackage itself has changed, we need to update the way we build our API docs. We need to build docs in each folder and make our own index page. No code has to change, and Typedoc uses its own TypeScript compiler version, so we're good to go. This will actually be an improvement since our current API landing page is awkward.

  • Use project references
  • Update our integrity script to populate references
  • Clean up watch mode in light of these simplifications
  • Fix handling of update:dependency foo@next
  • Audit all watch scripts - do not use -b in examples since they should be standalone
  • Clean up API docs handling
  • Add top level landing page
@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Sep 22, 2018

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Sep 25, 2018

This is now working using the nightly release of TypeScript, will do one more check with the final 3.1 release, which appears to be imminent.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Sep 25, 2018

Spoke too soon, still failing: microsoft/TypeScript#27032 (comment)

Copy link
Contributor

@jasongrout jasongrout left a comment

Works great for me - thanks!

@@ -220,6 +220,7 @@ export class ClientSession implements IClientSession {
this._name = options.name || '';
this._setBusy = options.setBusy;
this._kernelPreference = options.kernelPreference || {};
console.log('hi there again');
Copy link
Contributor

@jasongrout jasongrout Sep 27, 2018

We can take this out :)

@@ -87,6 +87,7 @@ function activateEditorCommands(
settingRegistry: ISettingRegistry
): void {
const { commands, restored } = app;
console.log('ha ha ha ha ha ha ha ha!');
Copy link
Contributor

@jasongrout jasongrout Sep 27, 2018

This could be removed too :)

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 27, 2018

Why did you move the tsconfig files into the src directories?

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Sep 28, 2018

Because otherwise you end up with lib/src/foo.js.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Sep 28, 2018

I'll finish the last couple issues flagged by Travis tomorrow morning, unless you beat me to it @jasongrout.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Sep 28, 2018

@meeseeksdev tag Status:Work in Progress

@meeseeksdev
Copy link

@meeseeksdev meeseeksdev bot commented Sep 28, 2018

Aww blink1073, I was not able to apply the following label(s): Status:Work in Progress. Either because they are not existing labels on this repository or because you do not have the permission to apply these.I tried my best to guess by looking at the casing, but was unable to find matching labels.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Sep 28, 2018

@meeseeksdev tag status:Work in Progress

@meeseeksdev
Copy link

@meeseeksdev meeseeksdev bot commented Sep 28, 2018

Aww blink1073, I was not able to apply the following label(s): status:Work in Progress. Either because they are not existing labels on this repository or because you do not have the permission to apply these.I tried my best to guess by looking at the casing, but was unable to find matching labels.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Sep 28, 2018

@meeseeksdev tag "status:Work in Progress"

@meeseeksdev
Copy link

@meeseeksdev meeseeksdev bot commented Sep 28, 2018

Aww blink1073, I was not able to apply the following label(s): "status:Work in Progress". Either because they are not existing labels on this repository or because you do not have the permission to apply these.I tried my best to guess by looking at the casing, but was unable to find matching labels.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Sep 28, 2018

@meeseeksdev tag status:Blocked

@meeseeksdev
Copy link

@meeseeksdev meeseeksdev bot commented Sep 28, 2018

Aww blink1073, I was not able to apply the following label(s): status:Blocked. Either because they are not existing labels on this repository or because you do not have the permission to apply these.I tried my best to guess by looking at the casing, but was unable to find matching labels.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Sep 28, 2018

cc @Carreau, is this a permissions issue, am I formatting wrong, or is this a blind spot in the parser?

@Carreau
Copy link
Contributor

@Carreau Carreau commented Sep 28, 2018

hum.. I've put some debu statement. let's see. It's definitively not a perm issue.

@meeseeksdev tag status:Blocked

@meeseeksdev
Copy link

@meeseeksdev meeseeksdev bot commented Sep 28, 2018

Aww Carreau, I was not able to apply the following label(s): status:Blocked. Either because they are not existing labels on this repository or because you do not have the permission to apply these.I tried my best to guess by looking at the casing, but was unable to find matching labels.

@Carreau
Copy link
Contributor

@Carreau Carreau commented Sep 28, 2018

Ah I think I know. Labels are pages, and it does not fetch all the labels.

@Carreau
Copy link
Contributor

@Carreau Carreau commented Sep 28, 2018

I've done an ugly fix, let's re-try

@meeseeksdev tag status:Blocked

@Carreau
Copy link
Contributor

@Carreau Carreau commented Sep 28, 2018

Should work if you have less than 200 potential tags :-) Easy to upgrade, but just to make sure there is no infinite loop.

Thanks for trying !

@meeseeksdev
Copy link

@meeseeksdev meeseeksdev bot commented Sep 28, 2018

tag all the things !

@@ -24,10 +24,11 @@
"url": "https://github.com/jupyterlab/jupyterlab.git"
},
"scripts": {
"build": "tsc",
"build": "tsc -b src",
Copy link
Contributor

@jasongrout jasongrout Sep 28, 2018

There are several places we have tsc -b src instead of tsc -p src. Did you mean to do -p instead of -b?

Copy link
Contributor

@jasongrout jasongrout Sep 28, 2018

Never mind, that was a naive question. The -b is one of the major points of the upgrade. What I didn't realize is that the -p was only left on test, build, and example projects, the projects without references.

Copy link
Contributor

@jasongrout jasongrout Sep 28, 2018

The test suite packages have -p, but also have references. Did you leave them with -p on purpose, or should they also be built with tsc -b?

Copy link
Member Author

@blink1073 blink1073 Sep 28, 2018

I left the handling of tests for the jest refactor.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 28, 2018

Tests pass now!

@jasongrout jasongrout merged commit 8a141ca into jupyterlab:master Sep 28, 2018
2 checks passed
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 28, 2018

Thanks again for all your work on this, @blink1073!

@jasongrout jasongrout changed the title [WIP] Use TypeScript 3.1 Use TypeScript 3.1 Sep 28, 2018
@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Sep 28, 2018

Thanks for helping it across the finish line!

@blink1073 blink1073 mentioned this pull request Sep 28, 2018
31 tasks
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 29, 2018

Because otherwise you end up with lib/src/foo.js.

You can fix that using the compiler rootDir option:

{
  "extends": "../../tsconfigbase",
  "compilerOptions": {
    "outDir": "lib",
    "rootDir": "src"
  },
  "include": ["src/*"]
}

While it's one more option in the tsconfig, doing it this way lets you drop the src on your references, like:

  "references": [
    {
      "path": "../apputils"
    },

and drop the -p/b src on most of the compilations.

I think treating the module itself as a ts project (rather than the src directory inside of the module) feels a bit cleaner also, in that you don't have so many .. directory backtracks and so many references to src/. Thoughts?

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Sep 29, 2018

Works for me, but let's wait until after my current jest PR

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 29, 2018

Works for me, but let's wait until after my current jest PR

Sounds good. One major change at a time...

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Sep 30, 2018

Incorporated into #5282 since I was touching those files anyway.

@blink1073 blink1073 deleted the use-ts-31 branch Oct 2, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants