feat(chunk): add chunk typing support #7031

TheLarkInn · 2018-04-13T00:17:06Z

What kind of change does this PR introduce?
Refactoring, Type Annotations

Did you add tests for your changes?
N/A

If relevant, link to documentation update:
N/A

N/A

Summary
This adds more (but not complete) typing to Chunk.js.

Chunk is one of many base Types that could value from being completely typed and referenced in other modules, etc. It is also heavily used across plugins in our ecosystem, and having these annotations will assist in authoring of plugins and other integrations.

Does this PR introduce a breaking change?
No

Other information

Also helped discover the following TypeScript bugs:

weswigham · 2018-04-13T21:41:58Z

lib/Chunk.js

@@ -388,7 +542,10 @@ class Chunk {
 		};
 	}

-	hasModuleInGraph(filterFn, filterChunkFn) {
+	hasModuleInGraph(
+		/** @type {(m: Module) => boolean} */ filterFn,


Would it be more consistent for these to be @param annotations on the method, instead of @type on the param?

Yeah I agree

mohsen1 · 2018-04-14T19:06:26Z

lib/util/SortableSet.js

@@ -1,5 +1,7 @@
 "use strict";
-
+//TODO: Make this a generic type


Should I just wait on this to land. Thats my thought.

I'll work on it tomorrow

sokra · 2018-04-15T07:33:50Z

lib/Chunk.js

 const getArray = set => Array.from(set);

+/**
+ * @param {Set} set the Set to get the count of
+ * @return {number} the count from the items in the set


count -> size

maby also change the variable name to reflect this.

sokra · 2018-04-13T18:45:45Z

lib/Chunk.js

+	//Blocked by https://github.com/Microsoft/TypeScript/issues/23375
+	/** @typedef {typeof import("./Module.js")} Module */
+	/** @typedef {typeof import("./ChunkGroup")} ChunkGroup */
+	/** @typedef {typeof import("./ModuleReason.js")} ModuleReason */


Move to top

sokra · 2018-04-13T18:48:20Z

lib/Chunk.js

@@ -312,7 +466,7 @@ class Chunk {
 		return this.addMultiplierAndOverhead(integratedModulesSize, options);
 	}

-	sortModules(sortByFn) {
+	sortModules(/** @type {(a: Module, b: Module) => -1|0|1} */ sortByFn) {


Use normal jsdoc.

Functions need to be declared as typedef to work with jsdoc

sokra · 2018-04-13T18:48:37Z

lib/Chunk.js

@@ -388,7 +542,10 @@ class Chunk {
 		};
 	}

-	hasModuleInGraph(filterFn, filterChunkFn) {
+	hasModuleInGraph(


sokra · 2018-04-15T07:36:20Z

lib/Chunk.js

 	constructor(name) {
 		this.id = null;
 		this.ids = null;
 		this.debugId = debugId++;
 		this.name = name;
 		this.entryModule = undefined;
+		//TODO make these typed generics for Module[] and ChunkGroup[] and their sort being (T, T): => 1,-1,0


sokra · 2018-04-15T07:36:44Z

lib/Chunk.js

 class Chunk {
+	/**
+	 * @param {string=} name of chunk being created, is optional (for subclasses)
+	 */
 	constructor(name) {
 		this.id = null;


every member should have a @type comment (and optionally @private)

webpack-bot · 2018-04-20T20:32:59Z

@TheLarkInn Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@sokra Please review the new changes.

sokra · 2018-04-23T06:53:38Z

lib/Chunk.js

@@ -12,18 +12,61 @@ const ERR_CHUNK_ENTRY = "Chunk.entry was removed. Use hasRuntime()";
 const ERR_CHUNK_INITIAL =
 	"Chunk.initial was removed. Use canBeInitial/isOnlyInitial()";

+//TODO: Use actual Module type instead of "faked"


This can be changed now?

You're waiting on microsoft/TypeScript#23570 to be reviewed, iterated, merged, and shipped. I think the associated issue's just marked fixed because we have a candidate fix ready.

It's merged now and will be in typescript@next tomorrow.

TheLarkInn · 2018-04-24T13:35:10Z

I have corrected import() statements with the new xx24 update of TypeScript nightly and the bug @weswigham mentioning being fixed last night.

sandersn · 2018-04-25T14:32:09Z

lib/Chunk.js

 	addMultiplierAndOverhead(size, options) {
 		const overhead =
-			typeof options.chunkOverhead === "number" ? options.chunkOverhead : 10000;
+			options.chunkOverhead === "number" ? options.chunkOverhead : 10000;


This removal of typeof looks incorrect. Is it?

oh yup this is the cause, thanks for seeing that! I hadn't gotten back to this yet. Thanks! Correcting.

TheLarkInn · 2018-04-28T05:52:42Z

This looks ready now.

TheLarkInn · 2018-04-28T17:36:49Z

Rebased to remove noise, tests rerunning, and removed two more stray comments. Should be good now.

webpack-bot · 2018-04-29T02:51:55Z

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

mohsen1 · 2018-04-29T05:01:45Z

lib/Chunk.js

+ */
+
+/** @typedef {Object} WithId an object who has an id property
+ *  @property {string} id - the id of the object


I think - prefix in description is extra

mohsen1 · 2018-04-29T05:03:07Z

lib/Chunk.js

 };

+/**
+ * @class


Lets remove extra @class, @name and @description directives for consistency.

mohsen1 · 2018-04-29T05:04:03Z

lib/Chunk.js

 	get entry() {
 		throw new Error(ERR_CHUNK_ENTRY);
 	}

+	/**
+	 * @deprecated .entry has been deprecated. Please use .hasRuntime() instead
+	 * @param {never} data The data that was attempting to be set


this is type any or * in JSDoc syntax

Actually never mind! I see what you're trying to do here! This is clever! Type system will complain if this setter is ever called.

Yup exactly!

mohsen1 · 2018-04-29T05:04:34Z

lib/Chunk.js

 		this.extraAsync = false;
 	}

+	/**
+	 * @deprecated Chunk.entry has been deprecated. Please use .hasRuntime() instead
+	 * @return {never} Throws an error trying to access this property


mohsen1 · 2018-04-29T05:06:12Z

lib/Chunk.js

@@ -121,42 +233,74 @@ class Chunk {
 		return false;
 	}

+	/**
+	 * @return {void} set new modules to this chunk and return nothing
+	 * @param {Module[]} modules the new modules to be set


Move @param to the top

TheLarkInn · 2018-05-01T03:25:54Z

Alright had to rebase, going to correct these now

TheLarkInn · 2018-05-02T17:18:13Z

@ooflorent @mohsen1 can I get re-reviews.

mohsen1 · 2018-05-02T17:32:10Z

lib/Chunk.js

+ */
+
+/**
+ * @typedef {(a: Module, b: Module) => -1|0|1} ModuleSortPredicate


nit: this can be single line

mohsen1 · 2018-05-02T17:33:10Z

lib/Chunk.js

@@ -34,51 +75,103 @@ const getModulesIdent = set => {
 	return str;
 };

+/**
+ * @param {Set} set the set to convert to array
+ * @returns {Array} the array returned from Array.from(set)


Does this have to be a generic?

TheLarkInn · 2018-05-03T23:17:39Z

Rebased master again, addressed @mohsen1's nits. Will wait on CI since we rebased just in case.

TheLarkInn · 2018-05-04T03:50:21Z

All checks pass going to merge.

Legends · 2018-06-12T13:30:59Z

lib/Chunk.js

 const sortByIdentifier = (a, b) => {
 	if (a.identifier() > b.identifier()) return 1;
 	if (a.identifier() < b.identifier()) return -1;
 	return 0;
 };

+/**
+ * @returns {string} a concatenation of module identifiers sorted
+ * @param {SortableSet} set to pull module identifiers from


@mohsen1 Cannot find the typedef for SortableSet. Is it not needed here?

It's imported on top of the file

webpack-bot added the PR: unreviewed label Apr 13, 2018

TheLarkInn changed the title ~~Feature/chunk type support~~ feat(chunk): add chunk typing support Apr 13, 2018

webpack-bot added the PR: CI-ok label Apr 13, 2018

weswigham reviewed Apr 13, 2018

View reviewed changes

mohsen1 mentioned this pull request Apr 14, 2018

(WIP) Enable TypeScript strictNullChecks #7037

Closed

6 tasks

mohsen1 reviewed Apr 14, 2018

View reviewed changes

sokra requested changes Apr 15, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested PR: review-outdated and removed PR: unreviewed PR: reviewed-changes-requested labels Apr 15, 2018

sokra closed this Apr 23, 2018

sokra reopened this Apr 23, 2018

sokra requested changes Apr 23, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: review-outdated labels Apr 23, 2018

sandersn mentioned this pull request Apr 23, 2018

module.exports = Entity is an alias, just like export = Entity microsoft/TypeScript#23570

Merged

webpack-bot added PR: conflict PR: review-outdated and removed PR: CI-ok PR: reviewed-changes-requested PR: conflict labels Apr 24, 2018

TheLarkInn closed this Apr 24, 2018

TheLarkInn reopened this Apr 24, 2018

webpack-bot added the PR: CI-not-ok label Apr 24, 2018

TheLarkInn mentioned this pull request Apr 24, 2018

chore(deps): update typescript nightly #7115

Merged

sandersn reviewed Apr 25, 2018

View reviewed changes

webpack-bot added the PR: conflict label Apr 28, 2018

webpack-bot removed PR: CI-not-ok PR: conflict labels Apr 28, 2018

TheLarkInn force-pushed the feature/chunk-type-support branch 3 times, most recently from d3df941 to 62d7737 Compare April 28, 2018 17:35

webpack-bot added the PR: CI-ok label Apr 29, 2018

mohsen1 reviewed Apr 29, 2018

View reviewed changes

lib/Chunk.js Outdated

};

/**

* @class

Copy link

Contributor

mohsen1 Apr 29, 2018

Choose a reason for hiding this comment

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

Lets remove extra @class, @name and @description directives for consistency.

mohsen1 reviewed Apr 29, 2018

View reviewed changes

sokra added the PR: types label Apr 30, 2018

mohsen1 approved these changes May 2, 2018

View reviewed changes

TheLarkInn force-pushed the feature/chunk-type-support branch 3 times, most recently from fd9b32e to ef3279e Compare May 3, 2018 23:16

chore(Chunk): add type annotations for Chunk

f1618ae

TheLarkInn force-pushed the feature/chunk-type-support branch from ef3279e to f1618ae Compare May 3, 2018 23:18

TheLarkInn merged commit e361ba5 into master May 4, 2018

montogeek deleted the feature/chunk-type-support branch May 4, 2018 08:11

Legends reviewed Jun 12, 2018

View reviewed changes

ahocevar mentioned this pull request Jun 15, 2018

Re-enable type checking openlayers/openlayers#8217

Closed

		@@ -1,5 +1,7 @@
		"use strict";

		//TODO: Make this a generic type

Uh oh!

feat(chunk): add chunk typing support #7031

feat(chunk): add chunk typing support #7031

Uh oh!

Conversation

TheLarkInn commented Apr 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webpack-bot commented Apr 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheLarkInn commented Apr 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheLarkInn commented Apr 28, 2018

Uh oh!

TheLarkInn commented Apr 28, 2018

Uh oh!

webpack-bot commented Apr 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheLarkInn commented May 1, 2018

Uh oh!

TheLarkInn commented May 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment