Refactor: No more than 1 namespace declaration per file #35373
Conversation
|
@typescript-bot pack this just to validate that renaming core.ts didn't break package generation or anything |
|
Heya @weswigham, I've started to run the tarball bundle task on this PR at 083bcd2. You can monitor the build here. It should now contribute to this PR's status checks. |
|
@typescript-bot pack this now that lint's fixed |
|
Heya @weswigham, I've started to run the tarball bundle task on this PR at 1698f40. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build. |
| @@ -0,0 +1,160 @@ | |||
| namespace ts { | |||
There was a problem hiding this comment.
x.ts/xPublic.ts is a fine naming scheme except that it makes bash-style completions worse because bu doesn't produce a filename.
One solution is to use a public- prefix, which also seems bad. Or maybe a subdirectory. I don't really have any good ideas. Maybe this situation is temporary? I hope?
There was a problem hiding this comment.
I like @sandersn 's idea of subfolder for internal. If we are not making subfolder i would like having xInternal and x so the file names exposes at smaller scale are nicer and without public
| /* eslint-disable no-var */ | ||
|
|
||
| // this will work in the browser via browserify | ||
| var _chai: typeof chai = require("chai"); |
There was a problem hiding this comment.
along the same lines, these files should probably be named global/languageService/utils.
There was a problem hiding this comment.
But this is all test harness setup...?
| // @ts-ignore | ||
| if (typeof process === "undefined" || process.browser) { | ||
| /// TODO: this is used by VS, clean this up on both sides of the interface | ||
| //@ts-ignore |
There was a problem hiding this comment.
mixed feelings about ts-ignore in ts file in Typescript codebase...
There was a problem hiding this comment.
Eh, we're doing things that are untyped here (and the typed "fix" is actually declaring these fields as globals, which I'd rather not do), so we need the bailout. It's also not our first - we already have one in the mapShim file to suppress an "unused type parameter" error (and is needed because said interface later merges with another in a dependent project where the param is actually used).
| /* @internal */ | ||
| namespace ts { | ||
| // These utilities are common to multiple language service features. | ||
| //#region |
There was a problem hiding this comment.
does VS Code support #region ??
| "utilities.ts", | ||
| "watchType.ts" |
This refactors our codebase to conform to and adds a lint rule to enforce a file having at most one top level namespace declaration. This makes the translation into modules much easier (and this refactor wasn't easily automatable, as some split namespaces existed to accomplish scope workarounds, which had to be done differently), as it means I don't have to track scope conflicts or what's exported from where when transforming the namespaces.
The primary effect of this is that a bunch of core utility files now have a clean split between their
@internaland public halves. Hopefully this'll have the beneficial side-effect of making it much more obvious what's public vs what isn't when you're authoring new utilities.This also slightly changes how we do some shim stuff in the harness, so that code will be more stable when it migrates from namespaces to modules - namely, no longer relying on
namespace Foomaking a globalFooto patch things into the global scope.