fix(rosetta): non-compiling snippets not reported on subsequent extracts

[kaizencc]

Picture this scenario: there is a non-compiling snippet in a README.md file.
I run yarn rosetta:extract --compile and it returns diagnostics as expected.
Behind the scenes, .jsii.tabl.json gets created as a cache, and this snippet
is inserted into that file with didCompile = false.

Without making any changes, I run yarn rosetta:extract --compile again.
This time, no diagnostics are returned, and it looks like my errors have
magically fixed themselves. However what is really happening is that extract
is finding a snippet in the cache that matches the offending snippet (since
I changed nothing). It is then filtering out that cached snippet, meaning we
do not actually try to compile the snippet again. This is bad; extract should
honor the --compile flag and return errors the second time around too.

There are two ways to solve this (that I can think of): we can return
diagnostics for cached snippets as well, or we can ignore non-compiling
cached snippets when --compile is set. I have opted for the second solution
for this reason: it is possible that I am intending to rerun the same snippet
with the expectation that I have changed something else that will result
in a successful compilation (for example, I add an import to the fixture).
Open to dialogue about this.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[rix0rrr]

Good catch!

for example, I add an import to the fixture

Well yes, but if you added an import to the fixture, that means the sample has changed and it would in fact get recompiled (try it).

You're definitely right that this is unexpected. In what cases do you suppose this is a very bad error? If you didn't treat the exit code of the first extract as a problem, but you DO treat the exit code of the second extract as a problem?

Not opposed to the solution... just trying to reason through the implications...

[kaizencc]

for example, I add an import to the fixture

Well yes, but if you added an import to the fixture, that means the sample has changed and it would in fact get recompiled (try it).

Welp, i was a bit iffy about whether this was true, should've tested it out first :).

The case I run into is when I am trying to get the README to compile, there are many errors at first. Say I work on a few of them and then run yarn rosetta:extract --compile again. I'm still expecting errors from the examples I didn't touch, but I just want to verify the few that I fixed are really fixed. However in this case the existing errors disappear, which is unexpected.

My solution is trying to take into account the experience of running rosetta extract without --compile. In this case we should want to match as much as possible from the cache regardless of whether they did compile. That makes me think we shouldn't be filtering when we first extract, since we don't know whether the second extract will come with a --compile flag or not. Does that make sense?

[rix0rrr]

Say I work on a few of them and then run yarn rosetta:extract --compile again.

Ah, iterative workflow! Yes of course, you are completely correct. It doesn't make sense to NOT display the same errors again. For some reason I was only thinking of the release build workflow.

I 100% agree with you.

[rix0rrr]

Conditional approval, I think we can do without the flag but otherwise I agree with the change.

[rix0rrr]

I would be fine making this the default. Let's not add switches we're not going to use.

[kaizencc]

I actually think the flag is necessary here. We pass in options.includeCompilerDiagnostics into compiledOnly. Without it, rosetta:extract --no-compile will not take advantage of the cache. For example:

run rosetta:extract --no-compile. examples added to tablet and cache.
run rosetta:extract --no-compile again. we have cached examples, but they are not used because didCompile is undefined. So we get cache misses for all snippets.

Now I don't know why one would do this. But as long as we support --no-compile it feels iffy to say "btw, you won't get to use the cache anymore, haha".

Unless you simply mean you want to default to compiledOnly = true?

[rix0rrr]

Holy shit it breaks my brain 😅.

I think you are right though, running through some cases and the alternatives feel suboptimal. Shipping this!

[mergify]

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

[mergify]

Merging (with squash)...