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

Remove declares in sys, core, evaluatorImpl #53176

Merged
merged 4 commits into from
Mar 10, 2023

Conversation

jakebailey
Copy link
Member

These all don't need to be there.

  • compiler references the node types, so we can do sys.ts and core.ts safely, which exposes some unsafety / redundant casts.
  • We have access to Symbol now due to the target bump, so no need to declare that either.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Mar 9, 2023
@@ -1507,7 +1501,7 @@ export let sys: System = (() => {
getAccessibleSortedChildDirectories: path => getAccessibleFileSystemEntries(path).directories,
realpath,
tscWatchFile: process.env.TSC_WATCHFILE,
useNonPollingWatchers: process.env.TSC_NONPOLLING_WATCHER,
useNonPollingWatchers: !!process.env.TSC_NONPOLLING_WATCHER,
Copy link
Member Author

Choose a reason for hiding this comment

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

This one is really suspicious; should we be parsing this rather than checking for truthiness? I made it !! to match current behavior, but, it seems unexpected.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sheetalkamat Do you have any advice for this particular place? Is this "fine" or was it intended that we let you set TSC_NONPOLLING_WATCHER=false or TSC_NONPOLLING_WATCHER=0 or TSC_NONPOLLING_WATCHER=off or similar?

Copy link
Member

Choose a reason for hiding this comment

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

I think its fine to fix it this way. This is from way back... given we have watchOptions now i dont think it matters. We probably should deprecate these environment variable options.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great, thanks!

The main benefit of the environment variables is being able to set them without modifying tsconfig, which is helpful if you're on a system where watch mode is broken and you don't want to commit a tsconfig change. (e.g. #52876 #52535)

@jakebailey jakebailey merged commit 42d2339 into microsoft:main Mar 10, 2023
@jakebailey jakebailey deleted the remove-random-declares branch March 10, 2023 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants