Skip to content

[uppercamelcase] add linting & enable strict null checks#18849

Merged
mhegazy merged 2 commits intoDefinitelyTyped:masterfrom
BendingBender:uppercamelcase
Aug 11, 2017
Merged

[uppercamelcase] add linting & enable strict null checks#18849
mhegazy merged 2 commits intoDefinitelyTyped:masterfrom
BendingBender:uppercamelcase

Conversation

@BendingBender
Copy link
Contributor

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: <>
  • Increase the version number in the header if appropriate.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

@dt-bot
Copy link
Member

dt-bot commented Aug 10, 2017

types/uppercamelcase/index.d.ts

to author (@plantain-00). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?


declare function upperCamelCase(...args: string[]): string;
export = upperCamelCase;
declare namespace upperCamelCase { }
Copy link
Contributor

@plantain-00 plantain-00 Aug 11, 2017

Choose a reason for hiding this comment

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

Why do you want to remove the namespace?
If the namespace is removed, the import * as upperCamelCase from "uppercamelcase"; will not work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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'));

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@plantain-00
Copy link
Contributor

👍
Seems for this lib, import * as ... syntax is not supported on purpose by typescript.

@typescript-bot
Copy link
Contributor

@plantain-00 - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

@plantain-00
Copy link
Contributor

👍
I searched awhile, and it seems a bug of: microsoft/TypeScript#17556
It may still take some time until the tagged v2.5 released.

@typescript-bot
Copy link
Contributor

Approved by a listed owner. PR appears ready to merge pending express review by a maintainer.

@mhegazy mhegazy merged commit 6167030 into DefinitelyTyped:master Aug 11, 2017
@BendingBender BendingBender deleted the uppercamelcase branch August 11, 2017 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants