Typescript type #3094

NejcZdovc · 2016-10-04T14:25:51Z

Please check if the PR fulfills these requirements

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

What is the current behavior? (You can also link to an open issue here)

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

If this PR contains a breaking change, please describe the following...

Impact:
Migration path for existing applications:
Github Issue(s) this is regarding:

Other information:
This resolves issue #3045.

marvinhagemeister · 2016-10-04T15:42:44Z

Great work! Out of curiosity, since I'm new to typescript: How do you run the tests? Is a simple compile enough?

NejcZdovc · 2016-10-04T16:23:55Z

@marvinhagemeister You can read more about here. You run test with a simple compile. It there are no errors like this

loader-test.ts(9,9): error TS2322: Type '{ entry: { alpha: string; beta: string; }; module: { loade1rs: { test: RegExp; loader: string; }[...' is not assignable to type 'Webpack'.
  Types of property 'module' are incompatible.
    Type '{ loade1rs: { test: RegExp; loader: string; }[]; }' is not assignable to type 'Module'.
      Object literal may only specify known properties, and 'loade1rs' does not exist in type 'Module'.

then definition is ok 😄

NejcZdovc · 2016-10-04T16:30:20Z

@TheLarkInn @sokra can you please check out TODO's in the code and tell me what to insert there.

quantizor · 2016-10-04T18:15:10Z

schemas/tests/agressive-merging-test.js

@@ -0,0 +1,19 @@
+/// <reference path="webpack.d.ts" />


aggressive (typo in the file name)

TheLarkInn · 2016-10-04T19:11:08Z

@gajus can you also assist here.

zhaoshengjun · 2016-10-05T01:55:47Z

schemas/webpack.d.ts

+    /*
+     Set the value of `require.amd` and `define.amd`.
+     */
+    amd?: any; // TODO missing type


Might change the comment on properties to JSDoc style. This enable better tooltip in editor. For exmaple

/** *Set the value of `require.amd` and `define.amd`. */

Good suggestion, changed.

zhaoshengjun · 2016-10-05T01:56:47Z

schemas/webpack.d.ts

+    /*
+     Set the value of `require.amd` and `define.amd`.
+     */
+    amd?: any; // TODO missing type


amd?: "require.amd" | "define.amd";

@zhaoshengjun I am not so sure about these two strings anymore. If you checkout documentation you can see there is an object example.

@TheLarkInn can you please clarify this property. Should be string literal here or object of some kind?

Starting tracing the use of the config value from here: https://github.com/webpack/webpack/blob/master/lib/WebpackOptionsApply.js#L248

To:
https://github.com/webpack/webpack/blob/master/lib/dependencies/AMDPlugin.js#L92-L100

It appears that the default is an empty Object, and that it is {[key: string]: boolean} if values are added. The key seems to represent the npm module/library name.

zhaoshengjun · 2016-10-05T02:09:08Z

schemas/webpack.d.ts

+        onlyModule?: boolean;
+    }
+
+    type EntryItem = string | string[];


If the type is only used in one place, I would suggest to do them inline. It's also for better editor tooltips.

It's not used only in one placed, that's why I added it to type.

entry: WebpackType.EntryItem | {[key: string]: WebpackType.EntryItem};

My fault, pointed to the wrong line.

I mean for OutputLibraryTarget , Stats and Target.

Personally I prefer to have this long list of enums in separate variable, so it's a lot more readable for me.

zhaoshengjun · 2016-10-05T02:26:53Z

schemas/webpack.d.ts

+     Boolean: You can pass `false` to disable it.
+     Object: You can pass an object to enable it and let webpack use the passed object as cache.This way you can share the cache object between multiple compiler calls.
+     */
+    cache?: (boolean | Object);


Suggest to add an Object interface. Reason is the current definition will let cache = "true" pass, which should not.

interface Object { [key: string]: any; }

After adding this, the above statement will cause error.

You are correct. Fixed

zhaoshengjun · 2016-10-05T03:41:36Z

Awesome work!

NejcZdovc · 2016-10-06T03:54:36Z

I finished with this task, so I am just waiting for you guys to fill out the blanks (where is TODO) and check if everything is in order. I will remove WIP tag from the title.

Jessidhia · 2016-10-06T05:01:45Z

schemas/webpack.d.ts

+        __filename?: boolean | "mock";
+        console?: boolean;
+        global?: boolean;
+        process?: boolean


Buffer, console and process can also be "mock" or "empty".

In scheme.json which is a base line for this PR it's says only boolean.

It's fixed now.

Jessidhia · 2016-10-06T05:04:30Z

schemas/webpack.d.ts

+        libraryTarget?: OutputLibraryTarget;
+
+        // The output directory as **absolute path** (required).
+        path?: string;


If it is required, it should not have a ?.

That's correct, but in the tests that you can find in this repository, there is a lot of example without path in output object, so I am not so sure about it. But on website it's required.

For now let's say it's required.

@TheLarkInn Please clarify this, because I see a lot of examples where path is not in configuration of output, but in documentation it says to be required.

Jessidhia · 2016-10-06T05:06:03Z

schemas/webpack.d.ts

+        root?: string;
+    }
+
+    interface OutputDevtoolLineToLine {


Would be nice if this (and everything else using test/include/exclude) would work like a RuleSetConditionObject, but that's outside this scope

I am not sure what you mean by that

They both have include, exclude and test keys, but they don't necessarily seem to work the same. Would be nice if both the implementation and the object format were the same.

Jessidhia · 2016-10-06T05:09:02Z

schemas/webpack.d.ts

+        | "node"
+        | "async-node"
+        | "node-webkit"
+        | "electron";


"atom" / "electron-main" are (possibly?) aliases to "electron". There is also a separate "electron-renderer" target.

As described here there are only these. Same is defined in the scheme.json.

I checked the source code itself; looks like an omission in the docs and scheme.json.

Let's create an issue for this for the JSON Schema so that once we fix it there, we fix it in both places.

Will do and create PR for schema

Created PR for it.

Done and added to the definition.

Jessidhia · 2016-10-06T05:10:56Z

schemas/webpack.d.ts

+     * A developer tool to enhance debugging.
+     * Note: Boolean can be used only with value `false`.
+     */
+    devtool?: string | boolean;


Would string | false work, or are constants in types only allowed for strings?

@mhegazy @DanielRosenwasser is there a better solution for this that you can think of?

string | false should be possible in TS 2.0

Thank you @DanielRosenwasser for update, I was relaying to much on IDE-a for checking correction of definition. Added.

NejcZdovc · 2016-10-06T06:03:49Z

@sokra In documentation there is <node buildin>: true, "mock", "empty" or false. Is this something that can be a single value for node property instead of object with options?

TheLarkInn · 2016-10-06T16:22:54Z

@sokra In documentation there is : true, "mock", "empty" or false. Is this something that can be a single value for node property instead of object with options?

I would say we conform exactly with the JSON Validation Schema. If we need to change this, we'll need to change it in both places. I want to keep these things in sync as much as possible.

ianks · 2016-10-07T18:02:38Z

Can these typings be added to the package.json?

TheLarkInn · 2016-10-07T19:23:41Z

@ianks yes definitely. @NejcZdovc @zhaoshengjun add a new field called "typings": and specify the path to that typings file.

Here's an example PR of someone doing it. jeffbcross/angularfire2@b3a5a3a

NejcZdovc · 2016-10-07T19:32:10Z

@TheLarkInn first I wanted to confirm location of definition file before adding it into package.json. So is the location correct or should we move it?

TheLarkInn · 2016-10-07T20:12:36Z

@DanielRosenwasser is there any general recommendation in placement for type definitions files in a package, that would contradict where we are creating it now in this PR.

DanielRosenwasser · 2016-10-07T20:21:21Z

In my opinion, the "schema" type should just be exposed as part of the webpack library .d.ts files, so that users can write something like

import webpack = require("webpack");

let config: webpack.WebpackConfiguration = {
   // ...
};

export = config;

NejcZdovc · 2016-10-08T03:34:53Z

@DanielRosenwasser I am curious why not add it to DT, import it as @type/webpack and add it to the package.json as typings. Then use it as let config: Webpack = {.

AFAIK this is the only .d.ts file that is part of webpack library. @TheLarkInn will there be more .d.ts files for example for plugins?

TheLarkInn · 2016-10-09T18:23:23Z

I think overall in the future if we transition this project to Typescript then each plugin technically will have a type. But for now we do not. I don't know if that answers the question for you.

TheLarkInn · 2016-10-11T22:39:54Z

#3112 had been merged so you can update accordingly

NejcZdovc · 2016-10-12T04:26:50Z

Updated accordingly of the #3112

TheLarkInn · 2016-10-12T04:49:39Z

schemas/webpack.d.ts

+    stats?: WebpackType.Object | boolean | WebpackType.Stats;
+
+    /**
+     * TODO add description


You could use something like this. I don't have good wording ideas for the second sentence, but essentially trying to capture what happens here

Specifies webpack deployment target. This modifies how the webpack bootstrap function is generated based on each target. Can also expose extra global variables into the webpack module scope (required for specific targets (IE: electron))

Specifies webpack deployment target. This modifies how the webpack bootstrap function is generated based on each target. Can also use predefined bootstrap functions based on targets such as electron, atom, etc.

What about something like that?

They are all predefined. So maybe that part is unnecessary (unless I'm misunderstanding).

I think:

Specifies webpack deployment target. This modifies how the webpack bootstrap function is generated based on each target.

Should be fine.

As far as I understand this is that you can specify string (predefined function as electron) or you can specify completely custom function, which is called here.

That's why we have: target?: Function | WebpackType.Target;

This is the reason why I formatted my sentence like this.

NejcZdovc · 2016-10-16T18:36:01Z

Ok, so AFAIK this is blocking merge of this PR

Position of this d.ts file
How will we use TS in reality (for now we have two proposals)

How can I help to resolve this two, so I can close this and go to another issue? @TheLarkInn @DanielRosenwasser

niieani · 2016-10-29T09:19:26Z

schemas/webpack.d.ts

+    type RuleSetRules = RuleSetRule[];
+}
+
+interface Webpack {


This interface should be exported, no? Doesn't make much sense having a type definition otherwise.

SimenB · 2016-12-24T09:52:51Z

Any plans to add matching flow type definitions?

EDIT: Saw the linked issue, but will there be any process to keep them in sync in the future? If TS is a part of this repo, but flow is not.

webpack-bot · 2017-08-11T11:41:59Z

@NejcZdovc The most important CI builds failed. This way your PR can't be merged.

Please take a look at the CI results from travis (error) and appveyor (failure) and fix these issues.

sokra · 2018-01-02T20:56:46Z

old, controversal, there is typing in DT, closing

quantizor reviewed Oct 4, 2016

View reviewed changes

NejcZdovc force-pushed the pr/#3045-definition branch from 045a284 to 386fe25 Compare October 4, 2016 18:46

zhaoshengjun reviewed Oct 5, 2016

View reviewed changes

NejcZdovc force-pushed the pr/#3045-definition branch from 386fe25 to ebf2008 Compare October 5, 2016 03:38

NejcZdovc changed the title ~~WIP: Typescript type~~ Typescript type Oct 6, 2016

Jessidhia reviewed Oct 6, 2016

View reviewed changes

NejcZdovc force-pushed the pr/#3045-definition branch from ebf2008 to c082fd7 Compare October 6, 2016 06:05

NejcZdovc mentioned this pull request Oct 9, 2016

Schema: Added missing properties #3112

Merged

11 tasks

NejcZdovc force-pushed the pr/#3045-definition branch 2 times, most recently from 1a6a4e7 to 57ac4e0 Compare October 12, 2016 04:26

NejcZdovc force-pushed the pr/#3045-definition branch from 57ac4e0 to 4e700c8 Compare October 12, 2016 04:39

TheLarkInn reviewed Oct 12, 2016

View reviewed changes

NejcZdovc force-pushed the pr/#3045-definition branch from 4e700c8 to f173a83 Compare October 13, 2016 04:54

Type definition with test

04953b6

NejcZdovc force-pushed the pr/#3045-definition branch from f173a83 to 04953b6 Compare October 13, 2016 07:12

niieani reviewed Oct 29, 2016

View reviewed changes

TheLarkInn mentioned this pull request Nov 29, 2016

[request] webpack flow-typed/flow-typed#165

Closed

webpack-bot added PR: CI-not-ok PR: unreviewed labels Aug 11, 2017

sokra closed this Jan 2, 2018

Typescript type #3094

Typescript type #3094

Conversation

NejcZdovc commented Oct 4, 2016 • edited Loading

marvinhagemeister commented Oct 4, 2016

NejcZdovc commented Oct 4, 2016 • edited Loading

NejcZdovc commented Oct 4, 2016 • edited Loading

quantizor Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheLarkInn commented Oct 4, 2016

Choose a reason for hiding this comment

NejcZdovc Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheLarkInn Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaoshengjun commented Oct 5, 2016

NejcZdovc commented Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jessidhia Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Jessidhia Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheLarkInn Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc commented Oct 6, 2016 • edited Loading

TheLarkInn commented Oct 6, 2016

ianks commented Oct 7, 2016

TheLarkInn commented Oct 7, 2016

NejcZdovc commented Oct 7, 2016

TheLarkInn commented Oct 7, 2016

DanielRosenwasser commented Oct 7, 2016 • edited Loading

NejcZdovc commented Oct 8, 2016 • edited Loading

TheLarkInn commented Oct 9, 2016

TheLarkInn commented Oct 11, 2016

NejcZdovc commented Oct 12, 2016

TheLarkInn Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

NejcZdovc commented Oct 16, 2016 • edited Loading

Choose a reason for hiding this comment

SimenB commented Dec 24, 2016 • edited Loading

webpack-bot commented Aug 11, 2017

sokra commented Jan 2, 2018

NejcZdovc commented Oct 4, 2016 •

edited

Loading

NejcZdovc commented Oct 4, 2016 •

edited

Loading

NejcZdovc commented Oct 4, 2016 •

edited

Loading

quantizor Oct 4, 2016 •

edited

Loading

NejcZdovc Oct 5, 2016 •

edited

Loading

TheLarkInn Oct 12, 2016 •

edited

Loading

NejcZdovc commented Oct 6, 2016 •

edited

Loading

Jessidhia Oct 6, 2016 •

edited

Loading

NejcZdovc Oct 6, 2016 •

edited

Loading

Jessidhia Oct 6, 2016 •

edited

Loading

TheLarkInn Oct 6, 2016 •

edited

Loading

NejcZdovc commented Oct 6, 2016 •

edited

Loading

DanielRosenwasser commented Oct 7, 2016 •

edited

Loading

NejcZdovc commented Oct 8, 2016 •

edited

Loading

TheLarkInn Oct 12, 2016 •

edited

Loading

NejcZdovc Oct 12, 2016 •

edited

Loading

NejcZdovc commented Oct 16, 2016 •

edited

Loading

SimenB commented Dec 24, 2016 •

edited

Loading