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

Upgrade typescript version #954

Merged
merged 5 commits into from
Jun 17, 2019
Merged

Upgrade typescript version #954

merged 5 commits into from
Jun 17, 2019

Conversation

fa93hws
Copy link
Contributor

@fa93hws fa93hws commented Jun 13, 2019

Latest version typescript has more strict type check.

src/servicesHost.ts Outdated Show resolved Hide resolved
@andrewbranch
Copy link
Contributor

Yeah, I don't think the loop is really worth it. Personally I’d probably go with something like

function addCache(servicesHost: typescript.ModuleResolutionHost): {
  moduleResolutionHost: typescript.ModuleResolutionHost,
  clearCache: () => void,
} {
  const clearCacheFunctions: Action[] = [];
  return {
    moduleResolutionHost: {
      ...servicesHost,
      fileExists: createCache(servicesHost.fileExists),
      directoryExists: servicesHost.directoryExists && createCache(servicesHost.directoryExists),
      realpath: servicesHost.realpath && createCache(servicesHost.realpath),
    },
    clearCache: () => clearCacheFunctions.forEach(clear => clear()),
  };

  function createCache<TOut>(originalFunction: (arg: string) => TOut) {
    const cache = new Map<string, TOut>();
    clearCacheFunctions.push(() => cache.clear());
    return function getCached(arg: string) {
      let res = cache.get(arg);
      if (res !== undefined) {
        return res;
      }

      res = originalFunction(arg);
      cache.set(arg, res);
      return res;
    };
  }
}

and then at the usage site

  let clearCache: null | (() => void) = null;
  let moduleResolutionHost: ModuleResolutionHost = {
    fileExists,
    readFile: readFileWithFallback,
    realpath: compiler.sys.realpath,
    directoryExists: compiler.sys.directoryExists,
    getCurrentDirectory: compiler.sys.getCurrentDirectory,
    getDirectories: compiler.sys.getDirectories
  };

  if (enableFileCaching) {
    const cached = addCache(moduleResolutionHost);
    clearCache = cached.clearCache;
    moduleResolutionHost = cached.moduleResolutionHost;
  }

I guess that’s slightly more wordy than what’s proposed here, but doesn’t contain any unsafe type assertions.

@johnnyreilly
Copy link
Member

Thanks for the suggestion @andrewbranch!

@fa93hws do you fancy tweaking your PR with the suggested implementation?

@andrewbranch
Copy link
Contributor

I feel like I should add that there are of course lots of different solutions that largely come down to personal preference of coding style. Avoiding unnecessary type assertions is a worthwhile goal, but I’m sure mine is not the most elegant possible solution.

@fa93hws
Copy link
Contributor Author

fa93hws commented Jun 17, 2019

No problem!

Copy link
Member

@johnnyreilly johnnyreilly left a comment

Choose a reason for hiding this comment

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

Looks good! Do you want to update the CHANGELOG.md? Remember to thank yourself 😁

@@ -88,7 +88,7 @@
"rimraf": "^2.6.2",
"tslint": "^5.11.0",
"tslint-config-prettier": "^1.15.0",
"typescript": "^3.1.1",
"typescript": "^3.5.1",
"webpack": "^4.5.0",
Copy link
Member

Choose a reason for hiding this comment

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

Shall we go for TypeScript 3.5.2 since it's out?

Copy link
Contributor Author

@fa93hws fa93hws Jun 17, 2019

Choose a reason for hiding this comment

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

No problem!
The version goes so quick

@johnnyreilly johnnyreilly merged commit 757137a into TypeStrong:master Jun 17, 2019
@johnnyreilly
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants