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

v2 #1348

Merged
merged 42 commits into from
Apr 19, 2018
Merged

v2 #1348

merged 42 commits into from
Apr 19, 2018

Conversation

Rich-Harris
Copy link
Member

here it comes...

@codecov-io
Copy link

codecov-io commented Apr 16, 2018

Codecov Report

Merging #1348 into master will decrease coverage by 0.16%.
The diff coverage is 91.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1348      +/-   ##
==========================================
- Coverage   91.69%   91.53%   -0.17%     
==========================================
  Files         127      127              
  Lines        4542     4464      -78     
  Branches     1446     1381      -65     
==========================================
- Hits         4165     4086      -79     
- Misses        151      153       +2     
+ Partials      226      225       -1
Impacted Files Coverage Δ
src/validate/html/index.ts 97.87% <ø> (ø) ⬆️
test/parser/index.js 100% <ø> (ø) ⬆️
src/parse/index.ts 84.81% <ø> (-2.69%) ⬇️
src/generators/nodes/shared/mungeAttribute.ts 92% <ø> (-0.31%) ⬇️
src/validate/html/validateHead.ts 60% <ø> (ø) ⬆️
src/parse/state/text.ts 100% <ø> (ø) ⬆️
src/generators/nodes/RawMustacheTag.ts 97.5% <0%> (ø) ⬆️
src/generators/nodes/Slot.ts 88.7% <0%> (-1.62%) ⬇️
src/validate/html/validateWindow.ts 88.23% <100%> (ø) ⬆️
src/generators/nodes/EachBlock.ts 98.42% <100%> (+0.78%) ⬆️
... and 39 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 84c146a...aaab685. Read the comment docs.

@@ -2,7 +2,7 @@ import assert from 'assert';
import fs from 'fs';
import { svelte, tryToLoadJson } from '../helpers.js';

describe('parse', () => {
describe.only('parse', () => {
Copy link
Member

Choose a reason for hiding this comment

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

D:

return base[0].toUpperCase() + base.slice(1);
}

describe("runtime", () => {
describe.only("runtime", () => {
Copy link
Member

Choose a reason for hiding this comment

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

D: D:

Copy link
Member Author

Choose a reason for hiding this comment

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

argh

@Rich-Harris
Copy link
Member Author

There's one really awkward failing test remaining. Trying to figure out the precise behaviour around initialisation and onstate. Bear with me while I think out loud.

It makes sense for onstate to fire before the main fragment is created: that way, any changes to the initial state object are reflected in the initial render.

During the initial render, it's possible for components with bindings to modify the parent state object. But we don't want onstate to refire for each binding when they are flushed, nor (I think) do we want computed properties to be recalculated each time. So we need some mechanism for coalescing state changes that result from bindings, and applying them between beforecreate and create.

(Note that I'm not describing a regression; the test in question was previously checking that bindings were flushed before oncreate, which remains the case. The difference is that we might now be able to prevent unnecessary recomputations.)

One valid option is to worry about this post-v2 (i.e. treat the current behaviour as a minor bug), if it's too hard to figure out right now.

@Rich-Harris
Copy link
Member Author

Rich-Harris commented Apr 16, 2018

I can't do any further work on this right now, but I believe there's just two remaining tasks:

  • error if element name is capitalised
  • rename loc property to start

@Rich-Harris Rich-Harris changed the title [WIP] v2 v2 Apr 17, 2018
@Rich-Harris
Copy link
Member Author

I think that's everything? Will prepare a PR for the docs site.

@Conduitry
Copy link
Member

The readme needs some updates - the structure of the return value of compile has changed, and references to removed compiler options should be removed.

@Conduitry
Copy link
Member

Conduitry commented Apr 18, 2018

The <svelte:window> special element creates some calls to .observe and to .get('foo') which need to be updated.

@Conduitry
Copy link
Member

Pushed a commit to hopefully fix the above issues with <svelte:window> but I'd definitely suggest taking a closer look at the changes first. It does seem that the new .on('state', ...) API lets us handle this more tidily.

window.scrollTo(window.pageXOffset, y);
window_updating_timeout = setTimeout(clear_window_updating, 100);
component.on("state", ({ changed, current }) => {
if (changed["y"]) {
Copy link
Member Author

Choose a reason for hiding this comment

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

any reason for preferring changed["y"] over changed.y?

Copy link
Member

Choose a reason for hiding this comment

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

Can we rely on the names of the bindings to be safe for use as property names? I was just trying to be super safe. If we can rely on that, or if there is some nice codegen utility function somewhere already that takes care of that, I'd support changing this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, I thought there was a utility for that but it seems we're just doing this everywhere instead...

generator.legacy ? `["default"]` : `.default`

(which reminds me — need to remove that)

@Rich-Harris Rich-Harris merged commit bba0444 into master Apr 19, 2018
@Rich-Harris Rich-Harris deleted the v2 branch April 19, 2018 12:54
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

3 participants