Use Typescript #425
Use Typescript #425
Conversation
|
Code Climate has analyzed commit c3b6d46 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 90.6% (0.0% change). View more on Code Climate. |
|
No clue why |
|
@BBosman There's probably a whole chunk of tests failing actually, I'm seeing this TS compiler error in the node tests: The browser tests will simply fail to load entirely if there is some error like a module import that cant be resolved which results in those tests not running (that's where the -1.5% coverage comes from). It's probably a good idea to turn on the type checker in webpack. Anyway, the node tests do go through normal typescript compilation so they will actually tell you what's wrong during compilation of the tests. Webpack completely suppresses test compiler errors with the current settings (this was done for speed) |
|
@fkleuver I did see that error, but I don't understand why it's giving it. The file export function $hydrateElement(this: Writable<ICustomElement>, flags: LifecycleFlags, parentContext: IServiceLocator, host: INode, options: IElementHydrationOptions = PLATFORM.emptyObject): void {
// ...
}And export function $hydrateElement(flags, parentContext, host, options = PLATFORM.emptyObject) {
// ...
}So it is there and being exported imho. It is missing from |
|
@BBosman It's marked internal so it doesn't end up in the type defs, that's why it's imported via a relative import |
|
@fkleuver That explains why it's missing in the (I tried removing the |
|
@BBosman there is a compiler error in |
|
@BBosman Looks like this was a regression in Now, suppressing that with We might actually want to keep |
| @@ -21,7 +21,7 @@ const slice = Array.prototype.slice; | |||
|
|
|||
| export function createElement<T extends INode = Node>( | |||
| dom: IDOM<T>, | |||
| tagOrType: string | Constructable, | |||
| tagOrType: string | Class, | |||
There was a problem hiding this comment.
Will this still work if you pass a regular function to it (I'm not even sure if that worked before, but I just got to think of it)
There was a problem hiding this comment.
No, it will give a type error. Changing it would have to be downstream as well, cause createElementForType has it as well.
Codecov Report
@@ Coverage Diff @@
## master #425 +/- ##
==========================================
+ Coverage 90.31% 90.31% +<.01%
==========================================
Files 139 139
Lines 11282 11286 +4
Branches 790 790
==========================================
+ Hits 10189 10193 +4
Misses 1093 1093
Continue to review full report at Codecov.
|
|
@fkleuver The I also reverted the
Isn't the purpose of an |
|
@BBosman Well, technically you can't pass in anything that's "invalid". There are no type constraints on what can be a custom element or custom attribute apart from it being constructable. What that method gives you is the ability to tell whether something is a certain resource type, and if it is, inside the Put simply, truly type checking the resources is too hard while TypeScript doesn't support stuff like decorator type merging and properly deriving prototypes, etc. It will go wrong too often and cause annoyances. I tried really really hard and spent several solid evenings on it. What's left is just a mess of stuff that didn't really turn out to work but I haven't had the time to clean up yet. |
|
@BBosman sorry for not following up for a bit, is there something you still needed help with on this PR? |
|
@fkleuver No problem. Life got in the way here, so haven't had time to look at it from my side either. I just rebased on master and pushed the most recent version (and updated the PR description). The only open issue is the failing The problem is that I don't know how to fix it, even with your (extended) explanation, so some hands on help would be appreciated. |
|
Minor update: Had to pin |
|
For starters let's separate dependency updates from refactors, because dependency updates generally don't need much review and could have been merged a long time ago. If the refactor depends on it and you don't want to have to wait for review/merge, it's fine if you create a new branch off the previous one and have a second PR with everything included. Then, once the first is merged, you can merge again and the diff will be normalized to only the latest changes automatically Do you think you can split it up so we can eliminate the deps from being a factor? |
|
@fkleuver Split of the dependency updates and the Unfortunately the problem still remains. I can fix it by using function isType<T>(this: ICustomElementResource, Type: T & { kind?: unknown }): Type is T & ICustomElementType { |
BBosman
force-pushed
the
typescript
branch
2 times, most recently
from
ed9cc26
to
d8fe56e
Compare
|
@fkleuver I've found a solution that I'm happy with and that works. If you could do another review when you've got a bit of time? |
|
@BBosman Overall it looks good but I'm not too fond of |
|
@fkleuver Valid comment. Did a bit of type refactoring and got rid of the |
|
Nice, thanks for fixing this up and apologies for the delay! |
Pull Request
π Description
Reap the benefits of the recent Typescript 3.3 update.
π« Issues
n/a
π©βπ» Reviewer Notes
Notable changes:
T & anyhack fromConstructable, as it's no longer needed.any, so I fixed those.π Test Plan
CircleCI
β Next Steps
n/a