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

internal: remove usage of hand crafted webpack typings #927

Merged
merged 4 commits into from May 1, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
Expand Up @@ -61,6 +61,7 @@
"@types/micromatch": "^3.1.0",
"@types/node": "^10.0.0",
"@types/semver": "^5.4.0",
"@types/webpack": "^4.4.27",
"babel": "^6.0.0",
"babel-core": "^6.0.0",
"babel-loader": "^7.0.0",
Expand Down
22 changes: 16 additions & 6 deletions src/after-compile.ts
@@ -1,12 +1,12 @@
import * as path from 'path';
import * as webpack from 'webpack';

import * as constants from './constants';
import { getEmitOutput } from './instances';
import {
TSFile,
TSFiles,
TSInstance,
WebpackCompilation,
WebpackError,
WebpackModule
} from './interfaces';
Expand All @@ -16,14 +16,24 @@ import {
isUsingProjectReferences
} from './utils';

interface PatchedCompiler extends webpack.Compiler {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bug #1 where the following properties are not available on a compiler instance.

isChild(): boolean;
context: string;
outputPath: string;
}

interface PatchedWebpackCompilation extends webpack.compilation.Compilation {
compiler: PatchedCompiler;
}

export function makeAfterCompile(
instance: TSInstance,
configFilePath: string | undefined
) {
let getCompilerOptionDiagnostics = true;
let checkAllFilesForErrors = true;

return (compilation: WebpackCompilation, callback: () => void) => {
return (compilation: PatchedWebpackCompilation, callback: () => void) => {
// Don't add errors for child compilations
if (compilation.compiler.isChild()) {
callback();
Expand Down Expand Up @@ -76,7 +86,7 @@ export function makeAfterCompile(
*/
function provideCompilerOptionDiagnosticErrorsToWebpack(
getCompilerOptionDiagnostics: boolean,
compilation: WebpackCompilation,
compilation: PatchedWebpackCompilation,
instance: TSInstance,
configFilePath: string | undefined
) {
Expand All @@ -103,7 +113,7 @@ function provideCompilerOptionDiagnosticErrorsToWebpack(
* this is used for quick-lookup when trying to find modules
* based on filepath
*/
function determineModules(compilation: WebpackCompilation) {
function determineModules(compilation: PatchedWebpackCompilation) {
// TODO: Convert to reduce
const modules = new Map<string, WebpackModule[]>();
compilation.modules.forEach(module => {
Expand Down Expand Up @@ -163,7 +173,7 @@ function determineFilesToCheckForErrors(
function provideErrorsToWebpack(
filesToCheckForErrors: TSFiles,
filesWithErrors: TSFiles,
compilation: WebpackCompilation,
compilation: PatchedWebpackCompilation,
modules: Map<string, WebpackModule[]>,
instance: TSInstance
) {
Expand Down Expand Up @@ -255,7 +265,7 @@ function provideErrorsToWebpack(
function provideDeclarationFilesToWebpack(
filesToCheckForErrors: TSFiles,
instance: TSInstance,
compilation: WebpackCompilation
compilation: PatchedWebpackCompilation
) {
for (const filePath of filesToCheckForErrors.keys()) {
if (filePath.match(constants.tsTsxRegex) === null) {
Expand Down
5 changes: 2 additions & 3 deletions src/compilerSetup.ts
@@ -1,7 +1,6 @@
import * as semver from 'semver';
import * as typescript from 'typescript';

import * as constants from './constants';
import { LoaderOptions } from './interfaces';
import * as logger from './logger';

Expand Down Expand Up @@ -66,9 +65,9 @@ export function getCompilerOptions(
if (
compilerOptions.module === undefined &&
(compilerOptions.target !== undefined &&
compilerOptions.target < constants.ScriptTargetES2015)
compilerOptions.target < typescript.ScriptTarget.ES2015)
) {
compilerOptions.module = constants.ModuleKindCommonJs;
compilerOptions.module = typescript.ModuleKind.CommonJS;
}

return compilerOptions;
Expand Down
6 changes: 4 additions & 2 deletions src/config.ts
@@ -1,7 +1,9 @@
import { Chalk } from 'chalk';
import * as path from 'path';
import * as typescript from 'typescript';
import { LoaderOptions, Webpack, WebpackError } from './interfaces';
import * as webpack from 'webpack';

import { LoaderOptions, WebpackError } from './interfaces';
import * as logger from './logger';
import { formatErrors } from './utils';

Expand All @@ -13,7 +15,7 @@ interface ConfigFile {
export function getConfigFile(
compiler: typeof typescript,
colors: Chalk,
loader: Webpack,
loader: webpack.loader.LoaderContext,
loaderOptions: LoaderOptions,
compilerCompatible: boolean,
log: logger.Logger,
Expand Down
4 changes: 0 additions & 4 deletions src/constants.ts
Expand Up @@ -7,10 +7,6 @@ export const LineFeed = '\n';
export const CarriageReturnLineFeedCode = 0;
export const LineFeedCode = 1;

export const ScriptTargetES2015 = 2;
Copy link
Member

Choose a reason for hiding this comment

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

So interestingly the moving of using locally defined constants to directly depending on TypeScript would mark the first direct runtime (rather than compilation time) dependency on the typescript module. All the others (I think) are type dependencies.

I believe, back in the mists of time, this was an intentional choice by ts-loader's @jbrantly. I think the reasons were to do with ts-loader working with multiple versions of TypeScript - not bolted to a specific one that you build with. It may been related to the ability to supply your own version of TypeScript. Remember ntypescript?

https://github.com/TypeStrong/ts-loader/blob/master/README.md#compiler-string-defaulttypescript

See also:

compiler = require(loaderOptions.compiler);

I don't know if this matters anymore. The values of those enums are pretty much set in stone now.

I'd appreciate any thoughts on this - it's probably fine. But I've a feeling that other views may be available too.

Also @arcanis - would this create issues for yarn PnP support?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also @arcanis - would this create issues for yarn PnP support?

Not if typescript is defined as either dependencies or peerDependencies (devDependencies won't do since they won't be considered part of the dependency tree when installed by consumers).

Copy link
Member

Choose a reason for hiding this comment

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

Cool - it's already a peerDependency so I guess we're good

"peerDependencies": {

  "peerDependencies": {
    "typescript": "*"
  }


export const ModuleKindCommonJs = 1;

export const extensionRegex = /\.[^.]+$/;
export const tsxRegex = /\.tsx$/i;
export const tsTsxRegex = /\.ts(x?)$/i;
Expand Down
28 changes: 13 additions & 15 deletions src/index.ts
@@ -1,18 +1,16 @@
import * as loaderUtils from 'loader-utils';
import * as path from 'path';
import * as typescript from 'typescript';
import * as webpack from 'webpack';

import * as constants from './constants';
import { getEmitOutput, getTypeScriptInstance } from './instances';
import {
AsyncCallback,
Compiler,
LoaderOptions,
LoaderOptionsCache,
LogLevel,
TSFile,
TSInstance,
Webpack
TSInstance
} from './interfaces';
import {
appendSuffixesIfMatch,
Expand All @@ -23,16 +21,16 @@ import {
validateSourceMapOncePerProject
} from './utils';

const webpackInstances: Compiler[] = [];
const webpackInstances: webpack.Compiler[] = [];
const loaderOptionsCache: LoaderOptionsCache = {};

/**
* The entry point for ts-loader
*/
function loader(this: Webpack, contents: string) {
function loader(this: webpack.loader.LoaderContext, contents: string) {
// tslint:disable-next-line:no-unused-expression strict-boolean-expressions
this.cacheable && this.cacheable();
const callback = this.async();
const callback = this.async() as webpack.loader.loaderCallback;
const options = getLoaderOptions(this);
const instanceOrError = getTypeScriptInstance(options, this);

Expand All @@ -51,9 +49,9 @@ function loader(this: Webpack, contents: string) {
}

function successLoader(
loaderContext: Webpack,
loaderContext: webpack.loader.LoaderContext,
contents: string,
callback: AsyncCallback,
callback: webpack.loader.loaderCallback,
options: LoaderOptions,
instance: TSInstance
) {
Expand Down Expand Up @@ -157,10 +155,10 @@ function makeSourceMapAndFinish(
outputText: string | undefined,
filePath: string,
contents: string,
loaderContext: Webpack,
loaderContext: webpack.loader.LoaderContext,
options: LoaderOptions,
fileVersion: number,
callback: AsyncCallback
callback: webpack.loader.loaderCallback
) {
if (outputText === null || outputText === undefined) {
const additionalGuidance =
Expand Down Expand Up @@ -198,7 +196,7 @@ function makeSourceMapAndFinish(
* either retrieves loader options from the cache
* or creates them, adds them to the cache and returns
*/
function getLoaderOptions(loaderContext: Webpack) {
function getLoaderOptions(loaderContext: webpack.loader.LoaderContext) {
// differentiate the TypeScript instance based on the webpack instance
let webpackIndex = webpackInstances.indexOf(loaderContext._compiler);
if (webpackIndex === -1) {
Expand Down Expand Up @@ -390,7 +388,7 @@ function getEmit(
rawFilePath: string,
filePath: string,
instance: TSInstance,
loaderContext: Webpack
loaderContext: webpack.loader.LoaderContext
) {
const outputFiles = getEmitOutput(instance, filePath);

Expand Down Expand Up @@ -460,7 +458,7 @@ function getTranspilationEmit(
fileName: string,
contents: string,
instance: TSInstance,
loaderContext: Webpack
loaderContext: webpack.loader.LoaderContext
) {
const {
outputText,
Expand Down Expand Up @@ -495,7 +493,7 @@ function makeSourceMap(
outputText: string,
filePath: string,
contents: string,
loaderContext: Webpack
loaderContext: webpack.loader.LoaderContext
) {
if (sourceMapText === undefined) {
return { output: outputText, sourceMap: undefined };
Expand Down
17 changes: 10 additions & 7 deletions src/instances.ts
Expand Up @@ -2,6 +2,7 @@ import chalk, { Chalk } from 'chalk';
import * as fs from 'fs';
import * as path from 'path';
import * as typescript from 'typescript';
import * as webpack from 'webpack';

import { makeAfterCompile } from './after-compile';
import { getCompiler, getCompilerOptions } from './compilerSetup';
Expand All @@ -13,7 +14,6 @@ import {
TSFiles,
TSInstance,
TSInstances,
Webpack,
WebpackError
} from './interfaces';
import * as logger from './logger';
Expand All @@ -29,6 +29,9 @@ import { makeWatchRun } from './watch-run';

const instances = {} as TSInstances;

// TODO: workaround for issues with webpack typings
type PatchedHookCallback = any;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bug #2 because of same same bug in #1 which makes the compiler property not match the webpack typings.


/**
* The loader is executed once for each file seen by webpack. However, we need to keep
* a persistent instance of TypeScript that contains all of the files in the program
Expand All @@ -38,7 +41,7 @@ const instances = {} as TSInstances;
*/
export function getTypeScriptInstance(
loaderOptions: LoaderOptions,
loader: Webpack
loader: webpack.loader.LoaderContext
): { instance?: TSInstance; error?: WebpackError } {
if (instances.hasOwnProperty(loaderOptions.instance)) {
const instance = instances[loaderOptions.instance];
Expand Down Expand Up @@ -67,7 +70,7 @@ export function getTypeScriptInstance(

function successfulTypeScriptInstance(
loaderOptions: LoaderOptions,
loader: Webpack,
loader: webpack.loader.LoaderContext,
log: logger.Logger,
colors: Chalk,
compiler: typeof typescript,
Expand Down Expand Up @@ -307,10 +310,10 @@ function successfulTypeScriptInstance(
);
}

loader._compiler.hooks.afterCompile.tapAsync(
'ts-loader',
makeAfterCompile(instance, configFilePath)
);
loader._compiler.hooks.afterCompile.tapAsync('ts-loader', makeAfterCompile(
instance,
configFilePath
) as PatchedHookCallback);
loader._compiler.hooks.watchRun.tapAsync('ts-loader', makeWatchRun(instance));

return { instance };
Expand Down