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

fixed #478 #518

Merged
merged 2 commits into from Feb 10, 2019

Conversation

Projects
None yet
3 participants
@stroncium
Copy link
Contributor

stroncium commented Feb 10, 2019

Essentially, going into link phase triggers additional cycle of interpolation(one outside of linking phase as usual, and one more inside) which leads to weird results.

The fix is pretty simple(and looking at the code, even thought of originally): switch mode to raw for the linking phase.

@kazupon Note:
Prior to this fix, 2 tests already failed on my setup:

  • test/unit/datetime.test.js:50 - looks like timezone mismatch
  • test/unit/number.test.js:58 - missing space

Not sure if those are known problems or if I should report them.

@pr-triage pr-triage bot added the PR: unreviewed label Feb 10, 2019

@stroncium

This comment has been minimized.

Copy link
Contributor Author

stroncium commented Feb 10, 2019

@kazupon also, quoting CircleCI, Warning: Your project is using CircleCI 1.0 and will stop building on March 15, 2019. Update remaining projects to CircleCI 2.0 to continue building.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 10, 2019

Codecov Report

Merging #518 into dev will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev     #518   +/-   ##
=======================================
  Coverage   96.75%   96.75%           
=======================================
  Files           9        9           
  Lines         679      679           
=======================================
  Hits          657      657           
  Misses         22       22
Impacted Files Coverage Δ
src/index.js 99.65% <100%> (ø) ⬆️

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 58a3377...9627d12. Read the comment docs.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 10, 2019

Codecov Report

Merging #518 into dev will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev     #518   +/-   ##
=======================================
  Coverage   96.75%   96.75%           
=======================================
  Files           9        9           
  Lines         679      679           
=======================================
  Hits          657      657           
  Misses         22       22
Impacted Files Coverage Δ
src/index.js 99.65% <100%> (ø) ⬆️

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 58a3377...d7aa68b. Read the comment docs.

@stroncium

This comment has been minimized.

Copy link
Contributor Author

stroncium commented Feb 10, 2019

Oh, and for the context:

  • link phase will treat raw by switching to string and disabling values for underlying phases
  • render phase will obviously still run after linking phase, handling all the interpolations
  • there are some more precise and invasive methods of fixing this, but I decided to stay on the side of least impact/changes, but if needed i can go deeper into it
  • I added a test to ensure links to links still work properly
@kazupon
Copy link
Owner

kazupon left a comment

Good job! 👍
Thanks!

@kazupon kazupon merged commit 469edd9 into kazupon:dev Feb 10, 2019

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 96.75%)
Details
codecov/project 96.75% (+0%) compared to 58a3377
Details

@stroncium stroncium deleted the stroncium:fix-478 branch Feb 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment