-
-
Notifications
You must be signed in to change notification settings - Fork 119
Refactor module in TypeScript #28
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
Code compiled from ts to js does not match xo-standard and cannot be configured.
index.ts
Outdated
@@ -0,0 +1,237 @@ | |||
const util = require('util'); | |||
|
|||
//const toString = Object.prototype.toString; |
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.
?
tsconfig.json
Outdated
{ | ||
"compileOnSave": true, | ||
"compilerOptions": { | ||
"target": "es6", |
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.
Just set it to es5
and we can use any ES6 feature, regardless of having to support Node.js 4.
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.
The problem is with new Set()
, which is perfectly used but typescript won't provide a polyfill for es5 during the transpilation, so I would suggest to ponyfill with array and Array.indexOf
or an associative 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.
Should work if you specify:
"lib": [
"es2015"
],
tsconfig.json
Outdated
"outDir": ".", | ||
"noImplicitAny": true, | ||
"suppressImplicitAnyIndexErrors": 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.
We should create a source map file too, for better debugging experience.
tsconfig.json
Outdated
], | ||
"exclude": [ | ||
"node_modules" | ||
] |
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.
Excluding node_modules
is already the default.
No worries |
Thanks for working on this @ltetzlaff :) |
index.ts
Outdated
}; | ||
} | ||
|
||
// Some few keywords are reserved |
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 you link to the TS issue here in a comment?
index.ts
Outdated
export const even = isAbsoluteMod2(0); | ||
export const odd = isAbsoluteMod2(1); | ||
|
||
const isWhiteSpaceString = (x: any) => is.string(x) && /\S/.test(x) === 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.
Let's use a better name than x
, since it will show up in auto-completion. We can use value
instead.
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.
for all methods?
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, anywhere x
is used now.
index.ts
Outdated
} | ||
|
||
if (is.array(range) && range.length === 2) { | ||
// TODO: Use spread operator here when targeting Node.js 6 or higher |
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.
When you target es5
in the tsconfig, you can fix this TODO.
.npmignore
Outdated
@@ -0,0 +1 @@ | |||
index.ts |
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 needed as we're using the files
property in package.json
Does it really make sense to export each method as an individual export? I'm just asking. Because without the import {string} from 'is';
...
string('🦄'); vs import is from 'is';
...
is.string('🦄'); You could, of course, do this, but I doubt most will: import {string as isString} from 'is';
...
isString('🦄'); |
I published a TSLint config which we can use. Just run {
"extends": "tslint-xo"
} And the following to "lint": "tslint --project .", |
tsconfig.json
Outdated
"declaration": true, | ||
"outDir": ".", | ||
"noImplicitAny": true, | ||
"suppressImplicitAnyIndexErrors": 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.
Why this?
tsconfig.json
Outdated
"alwaysStrict": true, | ||
"declaration": true, | ||
"outDir": ".", | ||
"noImplicitAny": 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.
This is not needed. It's included in the strict
option.
index.ts
Outdated
export const falsy = (x: any) => !x; | ||
|
||
// the tests are designed for Number.isNaN rather than global isNaN | ||
export const nan = (x: any) => is.number(x) && isNaN(Number(x)); |
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 did you change this from the original?
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.
Number.isNaN is currently not supported and isNaN() is typeguarded to only accept number
see microsoft/TypeScript#15149
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.
Ok. Maybe you can use // @ts-ignore
on it? https://blogs.msdn.microsoft.com/typescript/2017/10/31/announcing-typescript-2-6/
If not, at least link to the TS issue in a comment above.
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.
Went through the code and have added a first feedback round.
Some extra things.
I believe we should add the TS files to a directory called source
and use dist
as output directory. Will make it more easy to maintain in the future when extracting things to separate files.
We should also write the tests in TypeScript so that the entire codebase is TypeScript.
I also agree with @sindresorhus regarding the default export. We should go for import is from '@sindresorhus/is';
.
.gitignore
Outdated
@@ -1,2 +1,5 @@ | |||
node_modules | |||
yarn.lock | |||
|
|||
index.js | |||
index.d.ts |
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.
Add final newline
index.ts
Outdated
@@ -0,0 +1,237 @@ | |||
const util = require('util'); |
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 * as util from 'util';
index.ts
Outdated
|
||
//const toString = Object.prototype.toString; | ||
const getObjectType = (x: any) => toString.call(x).slice(8, -1) as string; | ||
const isOfType = (type: string) => (x: any) => typeof x === type; // eslint-disable-line valid-typeof |
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.
Remove eslint-disable-line
comment
index.ts
Outdated
const isObjectOfType = (type: string) => (x: any) => getObjectType(x) === type; | ||
|
||
//export default is | ||
function is (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.
Remove space before function paren
index.ts
Outdated
export const arrayBuffer = isObjectOfType('ArrayBuffer'); | ||
export const sharedArrayBuffer = isObjectOfType('SharedArrayBuffer'); | ||
|
||
export const truthy = (x: any) => !!x; // eslint-disable-line no-implicit-coercion |
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.
Remove eslint-disable-line
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.
@sindresorhus Would it be better to use Boolean(x)
here?
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
index.ts
Outdated
export const plainObject = (x: any) => { | ||
// From: https://github.com/sindresorhus/is-plain-obj/blob/master/index.js | ||
let prototype; | ||
// eslint-disable-next-line no-return-assign |
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.
Remove eslint-disable-next-line
comment.
index.ts
Outdated
// We have to use anonymous functions for the any() and all() methods | ||
// to get the arguments since we can't use rest parameters in node v4. | ||
export const any = function (predicate: any) { | ||
return predicateOnArray(Array.prototype.some, predicate, arguments); |
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.
TS supports rest parameters.
index.ts
Outdated
}; | ||
|
||
export const all = function (predicate: any) { | ||
return predicateOnArray(Array.prototype.every, predicate, arguments); |
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.
Same as above, you can use rest parameters.
tsconfig.json
Outdated
"exclude": [ | ||
"node_modules" | ||
] | ||
} |
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.
Add final newline
Hi guys, thanks a lot for pointing out those changes, I'm really glad to contribute here. Now there are several points that introduce some difficulties, for instance: is.classFails on classes because TS-es5 makes functions out of it. Examplevar Foo = /** @class */ (function () {
function Foo() {
}
return Foo;
}());
var classDeclarations = [
Foo,
/** @class */ (function (_super) {
__extends(Bar, _super);
function Bar() {
return _super !== null && _super.apply(this, arguments) || this;
}
return Bar;
}(Foo))
]; is.plainObjectFails on generators because TS-es5 makes plainObjects out of it. Examplevar __generator = (this && this.__generator) || function (thisArg, body) {
var _ = { label: 0, sent: function() { if (t[0] & 1) throw t[1]; return t[1]; }, trys: [], ops: [] }, f, y, t, g;
return g = { next: verb(0), "throw": verb(1), "return": verb(2) }, typeof Symbol === "function" && (g[Symbol.iterator] = function() { return this; }), g;
function verb(n) { return function (v) { return step([n, v]); }; }
function step(op) {
... is.generatorFunctionFails as well. Examplereturn __generator(this, function (_a) {
switch (_a.label) {
case 0: return [4 /*yield*/, 4];
case 1:
_a.sent();
return [2 /*return*/];
}
}); Tomorrow i'll get back to adding tslint-xo, thx for adding that Sindre 👍 |
.gitignore
Outdated
@@ -1,2 +1,4 @@ | |||
node_modules | |||
yarn.lock | |||
dist/**/*.js* | |||
dist/**/*.d.ts |
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 would just ignore the whole directory instead:
dist
Yay, next round: Node 4 Build fails:Destructuring
Should we drop jsdom and use something that supports Node 4? Do you maybe have anything specific in mind @sindresorhus ? Node 8 Build fails:Typescript builds async functions to return this:var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) {
return new (P || (P = Promise))(function (resolve, reject) {
function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } }
function rejected(value) { try { step(generator["throw"](value)); } catch (e) { reject(e); } }
function step(result) { result.done ? resolve(result.value) : new P(function (resolve) { resolve(result.value); }).then(fulfilled, rejected); }
step((generator = generator.apply(thisArg, _arguments || [])).next());
});
}; Should we drop the test or add an OR-condition that matches the typescript-generated? |
.gitignore
Outdated
@@ -1,2 +1,3 @@ | |||
node_modules | |||
yarn.lock | |||
dist/ |
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.
Add final newline. Just use dist
without /
.
I recommend installing an editorconfig plugin for your text editor. Things like final newlines will then be added automatically.
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.
Yea, thanks for the hint, was just about time to give my dotfiles a refresh 👍
package.json
Outdated
"lint": "tslint --project .", | ||
"build": "tsc", | ||
"test": "npm run lint && npm run build && ava dist/tests", | ||
"prepublish": "npm run build" | ||
}, | ||
"files": [ |
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.
Just set it to dist
.
@@ -0,0 +1,3 @@ | |||
{ | |||
"extends": "tslint-xo" | |||
} |
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.
Final newline
src/index.ts
Outdated
@@ -0,0 +1,231 @@ | |||
import * as util from 'util'; |
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.
Rename the directory from src
to source
. @sindresorhus and I agreed upon this name in another project.
tests/test.ts
Outdated
import test from 'ava'; | ||
import {jsdom} from 'jsdom'; | ||
import m from '.'; | ||
import * as util from 'util'; |
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'd rather put the tests
directory into the source
directory. This way, everything regarding code sits together. This also means that the index.ts
file is compiled to dist/index.js
which seems a little better than dist/src/index.js
.
The only thing that should be done then is in the prepublish
hook. Install del-cli
as dev dependency and set this as prepublish script.
npm run build && del dist/tests
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.
Agreed
tests/test.ts
Outdated
|
||
const document = jsdom(); | ||
const createDomElement = el => document.createElement(el); | ||
/* This ensures a certain method matches only the 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.
Make comment single line
tsconfig.json
Outdated
"compilerOptions": { | ||
/* es2015 requires moduleResolution and module to be set, | ||
see https://github.com/Microsoft/TypeScript/issues/8189 */ | ||
"target": "es2015", |
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.
@sindresorhus Might have missed this, but didn't we want to target ES5
?
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.
@SamVerschueren Can't: #28 (comment)
Don't upgrade jsdom. The existing version was outdated exactly for this reason, to keep Node.js 4 support. |
Just comment out the test until we can figure out a solution for it. |
@ltetzlaff Any thoughts on #28 (comment) ? |
src/index.ts
Outdated
|
||
export const promise = (value: any) => nativePromise(value) || hasPromiseAPI(value); | ||
|
||
// TODO: Change to use `isObjectOfType` once Node.js 6 or higher is targeted |
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 this be used now?
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.
no, the comment is still valid because the restricting factor is node runtime here
tests/test.ts
Outdated
['undefined', | ||
{is: m.undefined, fixtures: [undefined]} | ||
], ['null', | ||
{is: m.null_, fixtures: [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.
Why not m.null
? Isn't the whole purpose of Lines 225-229
in the source file to allow that?
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 does work when using it in js but Typescript can't find a type definition for that due to it being added via defineProperties 😞
I know it is not optimal, do you have a better idea or a method that merges the namespace with reserved keywords here?
Unfortunately @ts-ignore
did not work out in that case.
tests/test.ts
Outdated
const document = new jsdom.JSDOM().window.document; | ||
const createDomElement = (el: string) => document.createElement(el); | ||
|
||
interface Test {is(value: any): boolean; fixtures: 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.
spread this to
interface Test {
is: (value: any) => boolean;
fixtures: any[];
}
and add a new-line after that
tests/test.ts
Outdated
], ['symbol', | ||
{is: m.symbol, fixtures: [Symbol('🦄')]} | ||
], ['array', | ||
{is: m.array, fixtures: [[1, 2], new Array(2)]} // tslint:disable-line:prefer-array-literal |
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 are multiple fixtures and this is a single line.
I'm fine with single-lining if there's only one fixture, although I'd rather consistency overall,
Meaning that each type would look like:
['typename',
{ is: m.typename, fixtures: [
// there can be one or more fixtures here, but this would be consistent, as there are currently
// tests with one fixture which are one lined, but also some that're multiple fixtures but also one lined
]}
]
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.
Agreed, makes perfect sense
tests/test.ts
Outdated
|
||
if (testData === undefined) { | ||
t.fail('is.object not defined'); | ||
|
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.
remove this empty line
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.
The linter config insists on that empty line before return, sry 👍
Alright, I updated because I did not find typings for that version, but going after the date I found that package version |
Yes, I agree, I'd go for enabling both import is from 'is'; as well as import {is} from 'is'; |
Why? I prefer one way to do something. |
Since there is only one export shall we go for default export then? import is from 'is'; |
Either using the code from before the ts-refactors or writing something that will match both our runtime targets and still provides type/ide support by using type overloads
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.
Alright, I think I covered everything you reviewed, sorry that so many things came up 😅
Yes |
package.json
Outdated
}, | ||
"main": "dist", |
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.
Add "typings": "dist/index.d.ts"
here as well. Not sure if needed in the main
field though, but I always point to the index.js
file explicitely.
"main": "dist/index.js",
"typings": "dist/index.d.ts",
source/tests/test.ts
Outdated
import {jsdom} from 'jsdom'; | ||
import m from '.'; | ||
import m from '..'; // tslint:disable-line:import-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.
The rule is disabled in [email protected]
so you can remove the comment.
fixtures: any[]; | ||
} | ||
|
||
const types = new Map<string, Test>([ |
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 terms of readability, I would rather see it formatted like this
const types = new Map<string, Test>([
['undefined', {
is: m.undefined,
fixtures: [
undefined
]
}],
['null', {
is: m.null_,
fixtures: [
null
]
}],
['string', {
is: m.string,
fixtures: [
'🦄',
'hello world',
''
]
}]
// ...
]);
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.
Agreed
source/tests/test.ts
Outdated
// it's supposed to and none of the other methods' types | ||
const testType = (t, type, exclude) => { | ||
for (const [key, value] of types) { | ||
/* This ensures a certain method matches only the 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.
One-line comment
source/tests/test.ts
Outdated
|
||
const {is} = testData; | ||
|
||
types.forEach(({fixtures}, key) => { |
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.
Use a for..of
instead (like it was).
for (const [key, {fixtures}] of types) {
// TODO: Automatically exclude value types in other tests that we have in the current one.
// Could reduce the use of `exclude`.
if (exclude && exclude.indexOf(key) !== -1) {
continue;
}
const assert = key === type ? t.true.bind(t) : t.false.bind(t);
for (const fixture of fixtures) {
assert(is(fixture), `Value: ${util.inspect(fixture)}`);
}
}
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.
Also, there's the comment about the exclude, we can use types.filter
to get rid of it
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.
Also, there's the comment about the exclude, we can use types.filter
to get rid of it
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 unless we do Array.from(types).filter()
since types
is a Map, but feel free to do so, If you feel that that would be an improvement ✌️
source/tests/test.ts
Outdated
['undefined', | ||
{is: m.undefined, fixtures: [ | ||
['undefined', { | ||
is: m.undefined, fixtures: [ |
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.
Put fixtures
on a newline as well.
package.json
Outdated
}, | ||
"main": "dist/index", |
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.
dist/index.js
package.json
Outdated
}, | ||
"main": "dist/index", | ||
"types": "dist/index", |
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.
dist/index.d.ts
What's your preferred way of distributing this library?
|
distributing only pure js, declaration and source map |
Landed. Thanks for all the hard work you put into this @ltetzlaff. We really appreciate it 🙌 |
Amazing work @ltetzlaff, seriously. |
Thanks for your words, I really enjoyed contributing to you guys. 🎉 😃 |
According to #9 I setup the typescript compiler, added typings where appropriate, otherwise
any
and configured the ignores so that npm will containindex.d.ts
andindex.js
and githubindex.ts
.Due to
index.js
being "volatile" compiled code I had to drop xo (sorry Sindre) because the style of the compiled code can't be configuredAlso some keywords are reserved (
null
,function
,class
) and needed to be merged into the declaration viaObject.assign()
, see keywords issue.