-
-
Notifications
You must be signed in to change notification settings - Fork 645
feat(generators): add typescript support #532
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
Conversation
6b75f73 to
5d85a72
Compare
evenstensberg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to go over this again and swap any with specific types. Did a first review but it's too much of any here to comment them all. Do a console.log(typeof thisVal) if you're insecure about the type of the values you're adding.
packages/generators/add-generator.ts
Outdated
| const webpackDevServerSchema = require("webpack-dev-server/lib/optionsSchema.json"); | ||
| const PROP_TYPES = require("@webpack-cli/utils/prop-types"); | ||
| import glob from "glob-all"; | ||
| import inquirerAutoComplete from "inquirer-autocomplete-prompt"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import autoComplete
packages/generators/add-generator.ts
Outdated
| PROPS.filter(food => | ||
| food.toLowerCase().includes(input.toLowerCase()) | ||
| ) | ||
| PROPS.filter((food: string) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this named food? ^_^
packages/generators/add-generator.ts
Outdated
| let manualOrListInput = action => | ||
| Input("actionAnswer", `What do you want to add to ${action}?`); | ||
| public prompting() { | ||
| const done: any = this.async(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a promise that resolves to a boolean if I remember right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean something like?
const done: Promise<boolean>
Also, if it's a done is a promise, the compiler would fail all calls like done(). I think done is a function. Something like this should work:
const done: () => void | boolean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a promise http://yeoman.io/authoring/running-context.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the asynchronous API you’re relying upon doesn’t support promises, then you can rely on the legacy this.async() way. Calling this.async() will return a function to call once the task is done.
It's not a promise. It could be something like this:
const done: () => void | boolean|
|
||
| // first index indicates if it has a deep prop, 2nd indicates what kind of | ||
| let isDeepProp = [false, false]; | ||
| const isDeepProp: any[] = [false, false]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a bit naive to use any here when you can see the type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I tried boolean[] and Array<object | boolean> but both won't work because we also assign the values as boolean, string, object, array in different scopes. Some of the examples:
| isDeepProp[1] = answerToAction.actionAnswer; |
| if (isDeepProp[2].length === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const isDeepProp: [boolean, string | boolean | object | object[]]^This is a valid type declaration which allows to control specifically which element should be of which type. Even after declaring the second element to be one of the boolean, string, object or array of objects, the compiler fails. I think the complex reassignment logic we're doing inside the generator is making the compiler's type prediction to fail.
TS compiler works for simple logic like doing typeof var === "string" and then using it as string, the compiler infers it to be a string even if the type was string | boolean ...other types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add a interface for this if it's possible
packages/generators/add-generator.ts
Outdated
| action = "resolve"; | ||
| } | ||
| const webpackSchemaProp = webpackSchema.definitions[action]; | ||
| const webpackSchemaProp: any = webpackSchema.definitions[action]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoid using any here, just check the prop instead
| emit(event: string, ...args: any[]): boolean; | ||
| listenerCount(type: string): number; | ||
|
|
||
| async(): any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async is probably a promise
| prompt(opt: IPromptOptions | IPromptOptions[], callback: (answers: any) => void): void; | ||
| prompt<T>(opt: IPromptOptions | IPromptOptions[]): PromiseLike<T>; | ||
| log(message: string) : void; | ||
| npmInstall(packages: string[], options?: any, cb?: Function): any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not any
|
|
||
| appname: string; | ||
| gruntfile: IGruntFileStatic; | ||
| options: { [key: string]: any }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not any here
| choices?: any[] | ((answers: Object) => any); | ||
| default?: string | number | string[] | number[] | ((answers: Object) => (string | number | string[] | number[])); | ||
| validate?: ((input: any) => boolean | string); | ||
| filter?: ((input: any) => any); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
insecure about the use of any here
| registerTask(name:string, tasks:any):void; | ||
| insertVariable(name:string, value:any):void; | ||
| prependJavaScript(code:string):void; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these any values should be more type specific
|
Please update the template with the relevant information and link this PR to the original issue |
5d85a72 to
97c62f7
Compare
|
@dhruvdutt Thanks for your update. I labeled the Pull Request so reviewers will review it again. @ev1stensberg Please review the new changes. |
evenstensberg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you rebase and I'll have another look?
97c62f7 to
4561b57
Compare
4561b57 to
5ac0f2b
Compare
| configPath = null; | ||
| // end the generator stating webpack config not found or to specify the config | ||
| } | ||
| this.webpackOptions = require(configPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you'll need to do a json read on this, require's will try to resolve dependencies, which we don't need to install in order to run the ast transformation
| this.configuration.config.webpackOptions[propType] = null; | ||
| } | ||
| }) | ||
| .then(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void here _: void
| } | ||
|
|
||
| public prompting() { | ||
| const done: () => void | boolean = this.async(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(_)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do that then we'll have to pass something when we call done() at the end of generators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh okay, let's keep it then (:
| @@ -0,0 +1,4 @@ | |||
| declare module "*.json" { | |||
| const value: any; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not any, bool | num | object | array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, we have to use any here else it won't be able to read the json file. This is just json-loader because TS doesn't have inbuilt support for importing .json files.
We've defined types while assigning / using json values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC @TheLarkInn
packages/generators/utils/entry.ts
Outdated
| * @returns {Object} An Object that holds the answers given by the user, later used to scaffold | ||
| */ | ||
|
|
||
| export default function entry(self: any, answer: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generator instance on self
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As types don't get carried forward in functions, we had to define an explicit interface for Yeoman which could be used here. So, added IYeoman.
packages/generators/utils/entry.ts
Outdated
| const webpackEntryPoint: object = {}; | ||
| entryIdentifiers = multipleEntriesAnswer.multipleEntries.split(","); | ||
|
|
||
| function forEachPromise(entries, fn) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing types
packages/generators/utils/module.ts
Outdated
| loader: string; | ||
| options: { | ||
| plugins: string[]; | ||
| presets: any[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string
packages/webpack-scaffold/index.ts
Outdated
|
|
||
| interface IInquirerInput extends IScaffoldBaseObject { | ||
| validate?: Function; | ||
| validate?: (input: any) => string | boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
input is string
packages/webpack-scaffold/index.ts
Outdated
| } | ||
|
|
||
| export function InputValidate(name: string, message: string, cb: Function): IInquirerInput { | ||
| export function InputValidate(name: string, message: string, cb?: (input: any) => string | boolean): IInquirerInput { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
input is string
| message: string | ((answers: Object) => string); | ||
| choices?: any[] | ((answers: Object) => any); | ||
| default?: string | number | string[] | number[] | ((answers: Object) => (string | number | string[] | number[])); | ||
| validate?: ((input: any) => boolean | string); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string as input
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
| copyTemplateFiles, | ||
| templateFn | ||
| export default function addonGenerator( | ||
| prompts: any[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bet is that these are array of functions or promises, debug it to check
| } | ||
| const packager = getPackageManager(); | ||
| const packager: string = getPackageManager(); | ||
| const opts = packager === "yarn" ? { dev: true } : { "save-dev": true }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing type
| configPath = null; | ||
| // end the generator stating webpack config not found or to specify the config | ||
| } | ||
| this.webpackOptions = fs.readFileSync(configPath, "utf8"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you'll need to do a JSON parse too, this will be a utf8 encoded string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. This will return a string. Doing JSON.parse() on that string would result in error because the string contents aren't in JSON convertable format as it would be module.exports = { ... }. Even if we somehow read it in some JS variable, we would have to read object at module.exports.
I think the best way is to do require(configPath). Same as we were doing earlier. It would directly read the webpack config object present at module.export = { ... }.
We should always use require because the file we are reading is a node module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are right on the part on requiring the correct module, but when we do a require it will try to resolve dependencies we might not have in the repo and access variables that we might not need to bother with. It will result in a lot of errors for us and that's not good.
Try to implement this using a JSON read instead. It is fully possible. To access the variables you'll need to do something like module['entry'] etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will try to resolve dependencies we might not have in the repo.
require doesn't always / only resolve npm dependencies. It's also a standard convention for importing modules. If it's a file path like in this case, it won't try to resolve dependencies but simply import the module.exports object.
It will result in a lot of errors for us and that's not good.
We were already using this convention in the existing codebase (master).
| configModule = require(configPath); |
| ? require(resolvePluginsPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have some third opinion @ematipico ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, it should be changed. @dhruvdutt either way you do this, make sure not to import a file directly, as it would resolve the dependencies. I assume the closest thing you'll get to a error-free case is a stringified and parsed json
| type?: string; | ||
| } | ||
|
|
||
| export interface IWebpackOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These types need to be written as they will be, all of them are strings in the ast types, but here they are specified. Check webpack validation schema for definition of types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean everything from this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Base your values on that file, avoid using any
| @@ -0,0 +1,4 @@ | |||
| declare module "*.json" { | |||
| const value: any; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC @TheLarkInn
| public run(target: string, options?: object, done?: Function): IRunEnv; | ||
| public runInstall(packager: string, dependencies: string[], options?: object): void; | ||
| public on(event: string, listener: Function): this; | ||
| public async(): () => void | boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is _? void possible here?
| public run(target: string, options?: object, done?: Function): IRunEnv; | ||
| public runInstall(packager: string, dependencies: string[], options?: object): void; | ||
| public on(event: string, listener: Function): this; | ||
| public async(): () => void | boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is _?:void possible here?
tslint.json
Outdated
| "severity": "warning" | ||
| }, | ||
| "max-line-length": { | ||
| "options": { "limit": 120 }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's keep this to 70 and warn when it's over limit
0523834 to
67916d9
Compare
67916d9 to
fddd317
Compare
|
Needs a rebase :) |
evenstensberg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL
.gitignore
Outdated
| # Yarn lock file | ||
| yarn.lock | ||
|
|
||
| # Custom typings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing types folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decided to remove types from root level and move it inside packages. Removing it from gitignore was left.
| ), | ||
| ]) | ||
| .then(({ propType }) => { | ||
| .then(({ propType }: { propType: string }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
returns promise
| ]).then(({ keyType }) => { | ||
| Array.from(propValue), | ||
| ), | ||
| ]).then(({ keyType }: { keyType: string }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
returns promise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't returning anything inside this block. It should be void.
| ), | ||
| ]) | ||
| .then(({ keyType }) => { | ||
| .then(({ keyType }: { keyType: string }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
returns promise
| this.configuration.config.webpackOptions.module = { | ||
| rules: [ loader ] | ||
| }; | ||
| .then(({ rule }: { rule: string }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
returns promise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't returning anything inside this block. It should be void.
packages/generators/utils/entry.ts
Outdated
| ), | ||
| ]), | ||
| ).then((entryPropAnswer: object) => { | ||
| Object.keys(entryPropAnswer).forEach((val: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
returns void
packages/generators/utils/entry.ts
Outdated
| validate, | ||
| ), | ||
| ]), | ||
| ).then((entryPropAnswer: object) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
returns promise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually returning the entryPropAnswer itself / webpackEntryPoint which is an object.
packages/generators/utils/entry.ts
Outdated
| .then(singularEntryAnswer => { | ||
| .then((singularEntryAnswer: { | ||
| singularEntry: string, | ||
| }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
returns promise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually returning the singularEntry itself which is a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every promise returns a promise https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look at this code (from master) where we are returning non-Promise / string.
webpack-cli/packages/generators/utils/entry.js
Lines 88 to 97 in 617af0b
| .then(singularEntryAnswer => { | |
| let { singularEntry } = singularEntryAnswer; | |
| if (singularEntry.indexOf("\"") >= 0) { | |
| singularEntry = singularEntry.replace(/"/g, "'"); | |
| } | |
| if (singularEntry.length <= 0) { | |
| self.usingDefaults = true; | |
| } | |
| return singularEntry; | |
| }); |
There are many such examples where we aren't returning Promise. What should we do in such cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's some confusion here. The .then returns a promise but the actual callback called inside the .then returns whatever we want.
So if we have
Const p = promise.then(_ => 5)p is of the type Promise<Number> and the callback returns a type of Number.
To clarify, the callback doesn't return a promise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Emanuelle cleared it out. Add types for those though ;)
packages/generators/utils/plugins.ts
Outdated
| */ | ||
|
|
||
| module.exports = _ => { | ||
| export default function(): string[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(_?:void)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a promise, so _ should be removed. Can we have a eslint rule?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_ on empty params please http://po-ru.com/diary/rubys-magic-underscore/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's Ruby, that treats them differently, what's your point?
Also, underscore is used as temporary placeholder in libraries such as Ramda, where it has a different meaning. My point is, what's the heff meaning of this underscore in this project? Where's documented? Why there's no eslint rule to tell me that the developer has to put in certain cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Responded this internally. Needs an issue to be in the desc of the update docs issue
packages/webpack-scaffold/index.ts
Outdated
| type?: string; | ||
| name: string; | ||
| message: string; | ||
| choices?: ((answers: Object) => any) | any[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
returns array of strings
d001aff to
06de9e0
Compare
06de9e0 to
d7fc352
Compare
packages/generators/add-generator.ts
Outdated
| arr.forEach(p => { | ||
| const traverseAndGetProperties = (arr: object[], prop: string): boolean => { | ||
| let hasProp: boolean = false; | ||
| arr.forEach((p: object) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
returns void
packages/generators/add-generator.ts
Outdated
| PROPS.filter(food => | ||
| food.toLowerCase().includes(input.toLowerCase()) | ||
| ) | ||
| PROPS.filter((prop: string) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing ret type for filter
packages/generators/add-generator.ts
Outdated
| // Array -> Object -> Merge objects into one for compat in manualOrListInput | ||
| defOrPropDescription = Object.keys(defOrPropDescription[0].enum) | ||
| .map(p => { | ||
| .map((p: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing ret on map
packages/generators/add-generator.ts
Outdated
| }) | ||
| .reduce((result, currentObject) => { | ||
| for (let key in currentObject) { | ||
| .reduce((result: object, currentObject: object) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing return for reduce
packages/generators/add-generator.ts
Outdated
| "node_modules/webpack/lib/**/*Plugin.js", | ||
| ]) | ||
| .map(p => | ||
| .map((p: string) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map missing return
| ], | ||
| ["src/_index.js.tpl", "examples/simple/_webpack.config.js.tpl"], | ||
| gen => ({ name: _.upperFirst(_.camelCase(gen.props.name)) }) | ||
| (gen: IYeoman) => ({ name: _.upperFirst(_.camelCase(gen.props.name)) }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing return type
| if (this.webpackOptions.module && this.webpackOptions.module.rules) { | ||
| return this.webpackOptions.module.rules.map((rule: { | ||
| loader: string; | ||
| }) => rule ? rule.loader : null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing ret type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type is already present as an object.
packages/generators/utils/entry.ts
Outdated
| return promise.then((n: object) => { | ||
| if (n) { | ||
| Object.keys(n).forEach(val => { | ||
| Object.keys(n).forEach((val: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing return type
| * @returns {Function} A callable function that adds the babel-loader with env preset | ||
| */ | ||
| export default function(): IModule { | ||
| return { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
returns obj
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah! interface IModule extends Object :)
| */ | ||
| module.exports = value => { | ||
| export default function validate(value: string): string | boolean { | ||
| const pass = value.length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing type on const
d7fc352 to
7dad2d4
Compare
7dad2d4 to
75f018e
Compare
evenstensberg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
What kind of change does this PR introduce?
Did you add tests for your changes?
If relevant, did you update the documentation?
Summary
Does this PR introduce a breaking change?
Other information
Not ready for review yet.