Monaco.d.ts generation needs improvements

[joaomoreno]

For one, it's incredibly annoying when jumping between branches. It seems that I always have to restart gulp when switching branches due to monaco.d.ts (and now standaloneEnums.ts too). We can surely do better here.

Also, monaco.d.ts generation is hardly the most important thing in our build, why are its messages in ALL CAPS? Lets just make them play nicely with all our other messages.

On top of all of this, can we perhaps find another way to do this that doesn't involve committing computed changes? It seems to me that this would be much better implemented using a continuous build check.

[alexdima]

@joaomoreno The implementation of the monaco.d.ts generation has changed at commit 67eacaa. I have changed it last week to no longer rely on gulp-tsb and to allow us to use const enums in our code base. The positive effects are:

• the generation is faster (I wrote a custom formatter to do this)
• the overall build is faster (we are no longer generating .d.ts files for all of our source files)
• the product is faster at runtime (we can use const enums that get inlined even for enums which are exported).

The negative effects are:

• when switching to branches which do not contain 67eacaa, gulp watch must be restarted.

I'm sorry I covered this in great detail in the Zurich standup, but I guess I don't spend nearly enough time writing on Slack as I should be.

When switching branches in such a way that you go before 67eacaa, you need to restart gulp. I'm sorry about that, but that's just how this works. It is not possible to have this work in a backwards compatible way. It's like updating a node module Y to version N + 1 (which is incompatible with version N), then going back in time to source code which expects version N; when you execute things, things will explode unless you run yarn again.

TL;DR If switching branches in such a way that you are going back/forward in the past/future before 67eacaa you need to restart gulp watch. I am quite certain switching branches works correctly if not removing 67eacaa, i.e. if the branch is forked after 67eacaa or if 67eacaa is merged into it. So the branch switching is painful for as long as 67eacaa and previous related changes are not in the branch.

---

I can change the ALL CAPS messages.

---

Please feel free to propose a realistic working solution. I would love to not need to maintain a tree shaker and a custom .d.ts generation mechanism (with the goal of exposing only a subset of the source code as API). TypeScript is doing something similar with @internal annotations, but they are compiling everything to one output file to get a single .d.ts generated.

IMHO we are doing things the compiler should support: extracting a subset of our sources and a subset of classes/methods in those sources and calling them API in a .d.ts. Perhaps you can advocate this to the TypeScript team while you are in Redmond, I have tried to ask for help (microsoft/TypeScript#25831) and failed.

[alexdima]

I can build a version checking mechanism into the monaco.d.ts generation (in monaco.d.ts.recipe) to make the error message more clear and to make it even more clear that a gulp watch restart is mandatory.

To make a comparison closer to home, when we updated from TypeScript 2.x to TypeScript 3.x, if you were to start gulp watch on a version where TypeScript 2.x is loaded in memory, and then you switch branches to a version of vscode where we are already using TypeScript 3.x features, you would similarly need to restart gulp watch.

[alexdima]

Here are the improvements I pushed today:

• introduced a version number in monaco.d.ts.recipe. If the in-memory gulp watch loads with a different tooling version number, an error message is displayed inviting you to restart gulp watch.
• reduced the output generated by the monaco.d.ts generation both in the happy and in the error case
• changed the way .ts files are read to keep less initial state, i.e. be more tolerant to edits done to monaco.d.ts.recipe.
• changed the error message color to yellow to indicate that this is not a critical error.

If you have further ideas how to completely avoid the need for monaco.d.ts or how to do tree shaking / ship the standalone editor, please feel free to share them with me / Peng.

[joaomoreno]

@alexandrudima Thanks for writing this up and for the great improvements! cc @Microsoft/vscode

[joaomoreno]

The question is really about why is monaco.d.ts checked in? I know you have answered this to me, and I'm always forgetting... Is it because there are tools which fetch it directly from our GH repo or because you want people to be aware when they are changing editor API by having a modification marked in Git?

[alexdima]

Because it is part of the compilation and for example here the type monaco is used to ensure that we are really implementing and shipping what we are declaring we are shipping.

We use the same principle with the vscode API, we want that the implementation of the API is implementing the API and not something else.

[joaomoreno]

It seems to me, as @jrieken has pointed out before, that we got it right with vscode.d.ts. If it's being used as part of the compilation, we should probably be authoring it manually instead of generating and checking it into source.

In any case, good job, it seems pretty stable and quite fast now! 👏