[uppercamelcase] add linting & enable strict null checks#18849
[uppercamelcase] add linting & enable strict null checks#18849mhegazy merged 2 commits intoDefinitelyTyped:masterfrom
Conversation
|
types/uppercamelcase/index.d.ts to author (@plantain-00). Could you review this PR? Checklist
|
|
|
||
| declare function upperCamelCase(...args: string[]): string; | ||
| export = upperCamelCase; | ||
| declare namespace upperCamelCase { } |
There was a problem hiding this comment.
Why do you want to remove the namespace?
If the namespace is removed, the import * as upperCamelCase from "uppercamelcase"; will not work.
There was a problem hiding this comment.
Thank you for your review!
I was not aware that removing the namespace disables the import * as ... syntax. But to be honest, I'm not even sure, whether its worth it to keep this syntax. This syntax won't work in a native module-aware ES6 environment because ES6 module syntax doesn't allow you to export functions directly. So this is purely cosmetic and is even mentioned in the module-function template:
/*~ Note that ES6 modules cannot directly export callable functions.
*~ This file should be imported using the CommonJS-style:
*~ import x = require('someLibrary');
*~
*~ Refer to the documentation to understand common
*~ workarounds for this limitation of ES6 modules.
*/
Regarding the compilation, I don't really understand what you mean. I've set up a new project and tested it out, the compilation output looks the same regardless of whether the namespace is present or not:
"use strict";
exports.__esModule = true;
var upperCamelCase = require("uppercamelcase");
console.log(upperCamelCase('-asdas-asd'));There was a problem hiding this comment.
For code:
import upperCamelCase = require("uppercamelcase");
console.log(upperCamelCase("foo"));compiled with tsconfig.json:
{
"compilerOptions": {
"target": "es5",
"module": "esnext",
"moduleResolution": "node"
}
}the generated js file is:
console.log(upperCamelCase("foo"));You see, there is no var upperCamelCase = require("uppercamelcase"); in generated js file.
Why I use "module": "esnext" rather than "module": "commonjs"?
Because I use dynamic import(microsoft/TypeScript#14774 (comment)).
Why I use "moduleResolution": "node"?
Because I use angular(https://angular.io/guide/typescript-configuration#tsconfigjson), removing it will generate: Could not resolve module @angular/core.
So, how to make import foo = require() syntax work with angular and dynamic import?
There was a problem hiding this comment.
This is very confusing. Setting the module to es2015 leads to errors when using the import upperCamelCase = require('uppercamelcase'); syntax. Which I've expected. What I didn't expect is the output of esnext module system which lacks the require() statement altogether. What I know is that you can safely substitute import upperCamelCase = require('uppercamelcase'); with const upperCamelCase = require('uppercamelcase');.
But this change has the potential to break a lot of setups. I think the best way to proceed is to revert this change.
|
👍 |
…avoid a breaking change for users
|
@plantain-00 - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate? |
|
👍 |
|
Approved by a listed owner. PR appears ready to merge pending express review by a maintainer. |
npm run lint package-name(ortscif notslint.jsonis present).If changing an existing definition:
tslint.jsoncontaining{ "extends": "dtslint/dt.json" }.