-
Notifications
You must be signed in to change notification settings - Fork 263
Typescriptify - Conversion to TypeScript. #133
base: master
Are you sure you want to change the base?
Conversation
This file is supposed to allow checking that exactly the intended versions of dependencies are being used.
Removes the @debounce, @Throttle, @mixin, and @memoize decorators which have been deprecated for some time. The corresponding test cases have been removed as well, bringing the current number of tests expected to 56. BREAKING CHANGE: but we may not yet increment the major version! SemVer is currently not applicable to this package due to major version of 0.
Some edits needed, but nothing major. Left the hard ones as .js files.
This includes changin the output directories to cjs and esm.
They build into directory testBabel. With this change, we no longer need to use mocha switchs `--compilers js:babel-core/register` or `--require babel-polyfill`
Switched default output directory back to `lib` Switch clean to use del rather than rimraf Update VSCode launch & tasks config Fixup top-level tests for TypeScript
setup VSCode launch to test typescript
This reverts commit 4ae646f.
Typescript buidls with gulp-tsb for speed Integration with babel, eslint and mocha
There are a few warnings left.
Some of the TypeScript runs are still failing...
/built/ | ||
/esm/ | ||
/test/babel/ | ||
/test/typescript/ |
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.
Tests, from both transpilers, now build into these test
subdirectories so that they can be compared.
@@ -1,69 +0,0 @@ | |||
// Type definitions for core-decorators.js 0.19 |
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.
index.d.ts
is no longer need this because TypeScript is generating individual .d.ts
files per source file.
@@ -1,4 +0,0 @@ | |||
// index.ts - facade for core-decorators |
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.
Replaced index.ts/index.js with a change to main
setting in package.json
.editorconfig
Outdated
indent_size = 2 | ||
indent_style = space | ||
charset = utf-8 | ||
end_of_line = lf | ||
insert_final_newline = 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.
Ahh, I had the editorconfig add-in for vscode installed but disabled... That explains some things.
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 pretty big PR so it'll take me a bit to really dive deep into it and confirm, but I did an initial pass and semantically everything appeared great. I only found code style consistency issues.
src/core-decorators.ts
Outdated
export { default as lazyInitialize } from './lazy-initialize'; | ||
export { default as time } from './time'; | ||
export { default as extendDescriptor } from './extendDescriptor'; | ||
export { default as profile } from './profile'; | ||
|
||
// Exported for testing purposes | ||
export {default as defaultConsole} from './defaultConsole'; |
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.
please indent like the others.
export { default as defaultConsole } from './defaultConsole';
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.
Fixed
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.
P.S. I'd like to add tslint too, currently these escaped eslint because of the file extension.
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.
Groovy!
src/deprecate.ts
Outdated
return decorate(handleDescriptor, args); | ||
} | ||
|
||
export {deprecate, deprecate as deprecated} ; |
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.
indention
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.
Fixed
src/lazy-initialize.ts
Outdated
return decorate(handleDescriptor, args); | ||
} | ||
|
||
export {lazyInitialize}; |
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.
indention
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.
Fixed.
src/private/utils.ts
Outdated
throttleTrailingArgs = null; | ||
|
||
@lazyInitialize | ||
// @lazyInitialize |
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 this is extraneous?
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 remember commenting out the @lazyInitialize part was intentional. Its broken due to TypeScript's limitations on property initializers, even tests from Babel fail with the @lazyInitialize in place here.
As I remember removing the throttleTrailingArgs = null;
part was part of removing the obsolete decorators and their 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.
sorry if I'm misunderstanding but it's still not clear if this comment extraneous? It seems like it is
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! You mean I could simply delete the comment. Of course you are right.
src/profile.ts
Outdated
@@ -1,15 +1,9 @@ | |||
import { decorate, metaFor, warn, bind } from './private/utils'; | |||
import { decorate, metaFor } from './private/utils'; | |||
import {defaultConsole} from './defaultConsole'; |
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.
indention
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.
fixed
test/exports.spec.js
Outdated
const glob = require('glob'); | ||
const camelCase = require('camelCase'); | ||
const interopRequire = require('interop-require'); | ||
const decorators = require('core-decorators'); |
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 we lose the ability to use ESM imports? sorry if I missed a previous explanation.
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.
What I was shooting for was that the exports.spec.js and test.spec.js files should not require any transpiler. I think as long as we're running tests on a modern version of node.js, we could use ESM imports here, but I wasn't sure. I can restore them if you want. Please advise.
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.
@BurtHarris I wasn't going to burden you right now with it, but I was hoping to also switch the tests over to TypeScript so that we're actually also testing the type definitions at the same time. This can come later (if ever). For now your plan is OK with me, thanks for clarifying.
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.
@jayphelps I had shared your hope, just want to make sure you understand that as of TypeScript@2.4, the limited support for property decorators means we can't really test all of core-decorators from a TypeScript file. This is the quote from the TypeScript handbook that I think reflects the limitation:
NOTE A Property Descriptor is not provided as an argument to a property decorator due to how property decorators are initialized in TypeScript. This is because there is currently no mechanism to describe an instance property when defining members of a prototype, and no way to observe or modify the initializer for a property. As such, a property decorator can only be used to observe that a property of a specific name has been declared for a class.
I've tried to find issues tracking this restriction, microsoft/TypeScript#11866 seems one, but it's been closed, making reference to possibly reopening once the Public class fields TC39 proposal is ratified. I don't fully understand that proposal, but it seems different from the stage 3 decorators one. Can you see any reason they couldn't/shouldn't address it the way Babel does, by passing a synthetic (transpiler generated) descriptor to the first decorator in the list?
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 yes..I forgot they didn't support it. I say we punt.
Re TS updating to stage-3: I haven't heard, but I would bet money TS is waiting for the decorators proposal to be finalized stage-4 because in its current stage-3 form it's going to be quite the pain in the ass of a breaking change for the TS community aka mostly Angular people, so they want to be 100% sure of the changes so they only have to do it once. If it goes stage-4 fairly soon, there's not a lot of point in them adding the old initializer way that babel mostly just made up because the early spec didn't clarify
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.
But doesn't getting to stage 4 require "Two compatible implementations which pass the acceptance tests"? I was thinking TypeScript & Babel might be the likely implementations, have I misread the TC39 process and practices?
test/unit/override.spec.js
Outdated
@@ -36,7 +36,7 @@ describe('@override', function () { | |||
(function () { | |||
class Child extends Parent { | |||
@override | |||
speak(first, second) {} | |||
speak (first, second) {} |
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.
hmm why add the space in-between all of these? I'd prefer they weren't.
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.
Personally, I agree with you. The requirement for a space here is part of the eslint "semistandard" ruleset, and I'm not strongly advocating it.... As I said somewhere else, I picked that because it seemed a close match to existing code, but this whitespace was one change needed to conform to "semistandard".
I think picking a canned standard is easier than maintaining our own ruleset, but I don't have broad knowledge of all the rulesets out there.
Yes, sorry for the size of the PR @jayphelps. I've fixed the indentation issues. They slipped by because eslint doesn't understand typescript files, the long-term fix is to add tslint, but I didn't want to bloat the PR even more. I'll add a workitem to track that. On the space after function name issue, I agree with you. The requirement for a space there is part of the eslint "semistandard" ruleset. As I said somewhere else, I picked that ruleset because it seemed a close match to existing code, but this whitespace was one change needed to conform to "semistandard". I think picking a canned standard is easier than maintaining our own ruleset, but I don't have broad knowledge of all the rulesets out there. If you want to choose a different ruleset, or have me tweak that rule I'm happy to oblige. |
No need to apologize, by its nature it has to be huge--but I appreciate it.
I don't have a preference on standardized ruleset vs custom either way; I've always maintained my own, and IMO it's not a huge deal. I do however personally have a problem with the space in the CallExpressions, that we're talking about. If you'd prefer it, we can of course discuss it further, but if it's all the same to you let's not have the space. |
On the topic of code style, I think that the "recommended" style that comes with eslint may be a better match for both of our sensibilities. I'll try it out and update the branch. P.S. I wish there were a "do-not-merge" label I could attach to reflect the fact I still plan on touchups. Is there a way for me to add such a label? |
You should have permissions to create new labels and then add them to any tickets, if not lmk. That won't truly prevent you or I from merging it though. I've changed the settings now to disabling merge ability until at least one review approval. We'll see how it works. |
@jayphelps I've updated eslint to extend the "reccomended" set and added a rule for the function-parens spacing, fixing the violations. There are 15 lint warnings remaining, these are issues I'd like your input on at some time, particularly the |
@jayphelps My tests match up well with Appveyor, but Travis.CI seems to have other problems. I'm uncomfortable that neither of these were considered a failure and that the PR claims that "All checks have passed." Thoughts? If you are a Linux guy, perhaps you can look into what's happening on Travis.CI. I'll take a fresh look at the 10 TypeScript failures this weekend. |
Travis is reporting success because The separate issue that there's an error to begin with I commented inline on the problem (case sensitive |
test/exports.spec.js
Outdated
const chai = require('chai'); | ||
const path = require('path'); | ||
const glob = require('glob'); | ||
const camelCase = require('camelCase'); |
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 what is causing the error on Linux. This import string needs to be camelcase
all lowercase. Linux filesystem is case-sensitive.
Due to a complicated bug, npm is acting strange for me. I've found the Yarn package manager doesn't have the same prolem. Added a yarn.lock file, and a note in README.md. Having both a yarn and npm lock file seems like it might be undesirable long term, but we can reexamine this once some one of the npm/VSCode/typescript bugs are resolved.
Some tests have been marked so that they are skipped in typescript, but these show up as pending rather than failed. These include: - Two tests in autobind.spec.js (needs review) - Two in lazyIntialize.spec.json - One in readonly.spec.json - One in nonenmerable.spec.json
This matters on Linux.
62fb5b9
to
4533023
Compare
|
Unfortunately we may have jumped the gun here. Babel is actively working on adding support for stage-2 decorators and when they do, I think core-decorators needs to follow suit. AFAIK TypeScript has not yet offered any public indication of when (or even if) they're going to do so as well. Thoughts? |
One thing we could do, that I don't like (but this situation sucks no matter what) is indeed convert core-decorators to TypeScript, release it and continue to maintain it as normal then when babel gets stage-2+ spec we go back to babel but maintain the TypeScript version in a branch. We could continue to make bug fixes on it (bumping the semver patch on that same locked minor version). |
Just had another thought: I haven't looked into deeply yet, but it may be possible for us to detect which spec version is being used by the user simply by the arguments passed in. Duck-type checking or instanceof. If that's the case, we could still in fact write this in TypeScript and have it check at runtime which one is being used. The trouble is we need to test the Babel codepath, and I don't think it's possible for TypeScript to compile to something that would work in that case--but we might be able to get away with only using syntax extensions that are also supported by babel's Flow preset. |
Reviewing TypeScript PRs scheduled for inclusion in the next version I found microsoft/TypeScript#19675, which looks like it may be related the remaining issue blocking this. @jayphelps could you review the comments on that PR? If I get some cycles I'll try building this with TypeScript@next and verify. |
Readme still needs to be updated.
@debounce
,@throttle
,@mixin
, and@memoize
, and associated testsThe tests remain in ECMAScript, so that they can be run for both Babel and TypeScript generated versions. Some of the test cases aren't working in TypeScript yet, and may require a typescript upgrade to support them. But all the remaining tests continue to work when called from Babel processed sources.
Tests now require the
core-decorators
package by name, so they are like any dependent code. This is supported at test-time by creating a symlink in ./test/node_modules/core-decorators pointing to the project root directory.