Base scoping hashes on CSS content rather than entire file

[emilos]

Attempt to close #1091 :)

[codecov-io]

Codecov Report

Merging #1105 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1105      +/-   ##
==========================================
+ Coverage   91.56%   91.57%   +<.01%     
==========================================
  Files         123      123              
  Lines        4493     4496       +3     
  Branches     1447     1448       +1     
==========================================
+ Hits         4114     4117       +3     
  Misses        162      162              
  Partials      217      217

Impacted Files	Coverage Δ
src/parse/index.ts	89.65% <100%> (+0.36%)	⬆️

---

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 2537db9...f9fc7c9. Read the comment docs.

[Conduitry]

I don't think options.css being false is a reason to use parser.template. If options.css is false, that just means the user doesn't want to have code to inject the <style>, and they will make sure they get the styles another way.

If parser.css is null/undefined, I'm also not sure why we'd need to parse parser.template. In that case, doesn't Svelte just not add the extra attributes to any elements, and thus it doesn't need to have a hash? It seems like it might be possible to not have this getHashSource function and to below simply replace hash: hash(parser.template) with hash: parser.css && hash(parser.css.content.styles).

[emilos]

hey, thanks for the quick reply :)

I had a similar thought at first but I wasn't sure if hash value being set as null | undefined would be an expected value for a hash. Another reason for using the options.css flag is due to the test/runtime/samples/css-false spec, but I might've understood its intent in a wrong way.

After looking at it second time I'm not sure where the logic for the css-false spec sits, I'll try to investigate.

[emilos]

@Conduitry just fyi, I've investigated further and I've run into a weird scenario :) if I focus on given spec and change the code to:

hash: parser.css && hash(parser.css.content.styles)

it fails when all specs are run, but it passes correctly when the spec is focused with solo: true (it's not random and happens in 100% cases). Any ideas what could be happening? I guess it is happening since the tests are not isolated but I wasn't able to narrow down the issue yet

[Rich-Harris]

Thanks @emilos! I was wondering if we should keep the existing hash and add a new css.hash property — it seems more 'correct', and removes the risk of this being a breaking change for anyone. It also resolves the question about what to do when there's no CSS. What do you reckon?

[Conduitry]

In what way might this be a breaking change for anyone? The hash itself is never actually exposed in any publicly accessible API is it? Isn't it just internal?

edit: Oh, shit, forgot we exposed the parse function. Looks like that's not even in the readme though. I dunno. Hm.

[Rich-Harris]

Yeah, removing it isn't likely to cause problems, I just feel like it's easier that way. Otherwise I'd advocate for removing hash altogether in favour of css.hash (maybe that's the right thing to do? We could try it and see if anyone notices... or maybe replace hash with a getter that says 'please let us know what you're doing, because we're going to remove this property)

[emilos]

I found it a little bit confusing that the parser does return a hash that is used elsewhere. Right now it is used only in the Stylesheet.ts (at least in this codebase).

I think it would be most straightforward to add a new hash () {} method to the Stylesheet.ts and leave the parser part as is or remove it completely. That would probably make it a little bit more obvious which hash does the stylesheet actually use.

We could probably also add something like that to Template and Script as a mirror feature but I guess that's another topic and it's unnecessary for now :).

I'm fine with any option you guys decide is best.

Getting back to the question above, @Rich-Harris do you have any idea why the solo: true spec above passes alone but doesn't when whole suite has run?

[Rich-Harris]

Opened #1119 which bases the stylesheet.id property on the CSS contents alone, meaning parser.hash is basically unused. (Would have done it on this branch but not sure if that's possible? Can you only edit PRs via the web interface?)

Am baffled by the test failure, haven't been able to figure it out yet.

[Rich-Harris]

Ok, I figured it out — the CSS tests were leaving <style> tags in the DOM

[emilos]

@Rich-Harris cool 👍 thanks! I'll close this one then