search

jayphelps / core-decorators

Library of stage-0 JavaScript decorators (aka ES2016/ES7 decorators but not accurate) inspired by languages that come with built-ins like @​override, @​deprecate, @​autobind, @​mixin and more. Popular with React/Angular, but is framework agnostic.
MIT License
4.52k stars 263 forks source link

Typescriptify - Conversion to TypeScript. #133

Open BurtHarris opened 7 years ago

BurtHarris commented 7 years ago

Readme still needs to be updated.

The 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.

BurtHarris commented 7 years ago

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.

jayphelps commented 7 years ago

Yes, sorry for the size of the PR @jayphelps.

No need to apologize, by its nature it has to be huge--but I appreciate it.

If you want to choose a different ruleset, or have me tweak that rule I'm happy to oblige.

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.

BurtHarris commented 7 years ago

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?

jayphelps commented 7 years ago

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.

BurtHarris commented 7 years ago

@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 no-unused-expressions ones.

BurtHarris commented 7 years ago

@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.

jayphelps commented 7 years ago

Travis is reporting success because gulp test returned exit status 0, indicating success by POSIX convention. I'm not immediately familiar with gulp enough to know how to bubble up a non-zero exit status e.g. 1

The separate issue that there's an error to begin with I commented inline on the problem (case sensitive camelCase -> camelcase on linux_

BurtHarris commented 7 years ago

@jayphelps,

jayphelps commented 7 years ago

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?

jayphelps commented 7 years ago

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).

jayphelps commented 7 years ago

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.

BurtHarris commented 6 years ago

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.