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

Conversation

LukeSheard
Copy link
Contributor

@LukeSheard LukeSheard commented Apr 28, 2019

Some tidy up of the internals to use the @types/webpack webpack package for webpack specific typings.

As far as I can see there are two bugs in the webpack typings which I'll try and upstream: DefinitelyTyped/DefinitelyTyped#35053

@@ -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.

src/instances.ts Outdated
@@ -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.

@johnnyreilly
Copy link
Member

Nice! I've merged the changes in definitely typed... Let's see where this goes! 😀

@LukeSheard
Copy link
Contributor Author

LukeSheard commented Apr 28, 2019

Nice! I've merged the changes in definitely typed... Let's see where this goes! 😀

I think we just merged them in the wrong place facepalm

Moved to the correct place: DefinitelyTyped/DefinitelyTyped#35056

@johnnyreilly
Copy link
Member

Merged!

@LukeSheard
Copy link
Contributor Author

Updated the PR with the latest typings package! Passes locally for me, so should be good once Travis passes.

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.

Generally this looks amazing! Well done!

I've one question on the use of the TypeScript enums that I'd love you / @arcanis / @jbrantly to express views on before we proceed. But if everyone is happy then I'd ❤️ to get this merged!

if (existingModules.indexOf(module) === -1) {
existingModules.push(module);
function determineModules(compilation: webpack.compilation.Compilation) {
return compilation.modules.reduce<Map<string, WebpackModule[]>>(
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@@ -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": "*"
  }

@johnnyreilly
Copy link
Member

So I think we're good to merge and ship! Could you update the package.json and CHANGELOG.md please? We'll treat this as a patch I guess?

@LukeSheard
Copy link
Contributor Author

Will do!

@LukeSheard
Copy link
Contributor Author

Done!

@johnnyreilly johnnyreilly merged commit a6572ce into TypeStrong:master May 1, 2019
@johnnyreilly
Copy link
Member

Nice! Thanks for your work! ❤️

@johnnyreilly
Copy link
Member

Should be released shortly https://github.com/TypeStrong/ts-loader/releases/tag/v5.4.5

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