Skip to content
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

refactor(runtime): strictNullChecks fixes #336

Merged
merged 1 commit into from Dec 10, 2018

Conversation

BBosman
Copy link
Contributor

@BBosman BBosman commented Dec 8, 2018

Pull Request

πŸ“– Description

Refactoring runtime to adhere more to strictNullChecks.

🎫 Issues

Part of #254.

πŸ‘©β€πŸ’» Reviewer Notes

This is just a partial fix, but I decided to get some of the low hanging fruit out of the way first, before going for the bigger changes.

πŸ“‘ Test Plan

Trust in CircleCI.

⏭ Next Steps

Do the rest and turn the options on by default in the tsconfig files.

@codeclimate
Copy link

codeclimate bot commented Dec 8, 2018

Code Climate has analyzed commit b000aa2 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 92.5% (0.0% change).

View more on Code Climate.

@codecov
Copy link

codecov bot commented Dec 8, 2018

Codecov Report

Merging #336 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #336      +/-   ##
==========================================
- Coverage   92.03%   92.03%   -0.01%     
==========================================
  Files          81       81              
  Lines        7975     7972       -3     
  Branches     1295     1295              
==========================================
- Hits         7340     7337       -3     
  Misses        635      635
Impacted Files Coverage Ξ”
packages/runtime/src/templating/resources/with.ts 100% <ΓΈ> (ΓΈ) ⬆️
packages/runtime/src/lifecycle.ts 87.19% <ΓΈ> (ΓΈ) ⬆️
packages/runtime/src/binding/ast.ts 91.61% <100%> (ΓΈ) ⬆️
packages/runtime/src/resource.ts 100% <100%> (ΓΈ) ⬆️
packages/runtime/src/templating/view.ts 97.84% <100%> (ΓΈ) ⬆️
packages/runtime/src/definitions.ts 95.34% <100%> (ΓΈ) ⬆️
...ackages/runtime/src/templating/resources/repeat.ts 100% <100%> (ΓΈ) ⬆️
packages/runtime/src/templating/resources/if.ts 85.96% <100%> (ΓΈ) ⬆️
packages/runtime/src/dom.ts 95.38% <100%> (ΓΈ) ⬆️
...ackages/runtime/src/templating/lifecycle-render.ts 92.87% <100%> (ΓΈ) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data
Powered by Codecov. Last update 21b3f46...b000aa2. Read the comment docs.

Copy link
Member

@fkleuver fkleuver left a comment

Choose a reason for hiding this comment

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

LGTM

this.isMulti = expressions.length > 1;
this.firstExpression = expressions[0];
this.isMulti = this.expressions.length > 1;
this.firstExpression = this.expressions[0];
Copy link
Member

Choose a reason for hiding this comment

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

Imo, it should be

expressions = expressions || PLATFORM.emptyArray;

Copy link
Member

Choose a reason for hiding this comment

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

The API contract specifies "array or undefined", why use (slower) coercion instead of strict equality?

Copy link
Member

Choose a reason for hiding this comment

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

We still need some Minimizer friendliness. So it can be still strict, just with expression

Copy link
Member

Choose a reason for hiding this comment

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

I think we can tackle minimizer friendliness with AOT. There are a lot of parts in the framework that still have a long way to go in this area.

@EisenbergEffect EisenbergEffect merged commit a7bbfd6 into aurelia:master Dec 10, 2018
@BBosman BBosman deleted the sncruntime branch December 10, 2018 09:18
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.

None yet

4 participants