Skip to content

Add typings for library template plugins#7251

Merged
sokra merged 3 commits intomasterfrom
types/library_templates
May 28, 2018
Merged

Add typings for library template plugins#7251
sokra merged 3 commits intomasterfrom
types/library_templates

Conversation

@ooflorent
Copy link
Copy Markdown
Contributor

What kind of change does this PR introduce?

typings

Did you add tests for your changes?

no

If relevant, link to documentation update:

n/a

Summary

Add typings for library template plugins

Does this PR introduce a breaking change?

no

/** @typedef {import("./Compilation")} Compilation */

/**
* @param {string[]} accessor the accessor to convert to path
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ReadonlyArray<string>


class ExportPropertyMainTemplatePlugin {
/**
* @param {string|string[]} property the name of the property to export
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This class doesn't write to it so this can be string|ReadonlyArray<string> too

*/
apply(compilation) {
const { mainTemplate, chunkTemplate } = compilation;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

onRenderWithEntry is missing types (I guess you're more focused on public API for now?)

/** @typedef {import("./Compiler")} Compiler */

/**
* @param {string[]} accessor the accessor to convert to path
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ReadonlyArray<string>

if (idx === accessor.length - 1) return a;
/**
* @param {string=} base the path prefix
* @param {string|string[]} accessor the accessor
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is where I'd tell you to use string|ReadonlyArray<string> too but this would break Array.isArray. I recommend adding this to a global .d.ts, which will make Array.isArray support both ReadonlyArray and regular arrays:

interface ArrayConstructor {
  isArray(arg: any): arg is ReadonlyArray<any>;
}

I actually sent this as a PR to TypeScript, but it was rejected on compatibility grounds.

/** @typedef {import("./Compilation")} Compilation */

/**
* @param {string[]} accessor the accessor to convert to path
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ReadonlyArray<string>

if (idx === accessor.length - 1) return a;
/**
* @param {string=} base the path prefix
* @param {string|string[]} accessor the accessor
Copy link
Copy Markdown
Member

@Jessidhia Jessidhia May 10, 2018

Choose a reason for hiding this comment

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

Same problem here as in LibraryTemplatePlugin

(actually, should these functions be put in a common module?)

@webpack-bot
Copy link
Copy Markdown
Contributor

webpack-bot commented May 16, 2018

For maintainers only:

  • This need to be documented (issue in webpack/webpack.js.org will be filed when merged)

Copy link
Copy Markdown
Member

@sokra sokra left a comment

Choose a reason for hiding this comment

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

I would accept this. The comments about ReadonlyArray are true, but seems like typescript still has issue with it. This can be done in a separate PR when TS issues are fixed.

@webpack-bot
Copy link
Copy Markdown
Contributor

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

@sokra sokra merged commit 5c51f0c into master May 28, 2018
@sokra
Copy link
Copy Markdown
Member

sokra commented May 28, 2018

Thank

@sokra sokra deleted the types/library_templates branch May 28, 2018 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants