Conversation
jstrieb
left a comment
jstrieb
left a comment
There was a problem hiding this comment.
I'm going to update the CI pipeline to run on pull requests. (Haven't had any, so didn't need to think about that until now.) I'm sure you ran tests locally, but I'm not going to merge until CI runs on this, even if it's otherwise done.
src/App.svelte
Outdated
| load(urlData) ?? new State([new Sheet("Sheet 1", 10, 10)]), | ||
| ); | ||
| let globals; | ||
| globals = load(urlData) ?? new State([new Sheet("Sheet 1", 10, 10)]); |
There was a problem hiding this comment.
Change the constructor to not take Sheet instances, or else handle them specially. See comment below.
| let result = new State( | ||
| data.sheets.map((sheet) => { | ||
| constructor(sheets, formulaCode) { | ||
| this.load({ sheets, formulaCode }); |
There was a problem hiding this comment.
I think this happens to work, but isn't quite correct. We can either change the constructor, or change what we pass to it. (I lean towards the latter.)
Right now, the constructor is called on Sheet instances. In a Sheet instance, the cell values are rederivable stores. In State.load, those rederivable stores are being passed as the values for each cell. Instead, the values are supposed to be raw values, not stores. Probably as soon as the formula is evaluated the raw values are thrown out in all of the test cases (except self-reference), which is why this wasn't caught by the tests. This is the problematic line:
I think the best fix is to never pass fully initialized Sheet instances to the constructor like we're currently doing elsewhere. But I am open to other ideas.
There was a problem hiding this comment.
Fun fact: TypeScript would probably have caught this. Gonna have to set that up at some point...
There was a problem hiding this comment.
Ah good catch, thank you.
Fun fact: TypeScript would probably have caught this. Gonna have to set that up at some point...
Yeah probably. Also consider jsdoc if you don't want to deal with the transpiling, but then again svelte has a compilation step anyway
| test("State.load preserves elements", async () => { | ||
| const state = createSheet([["1", "2", "3"]]); | ||
| await expectSheet(state.currentSheet, [[1, 2, 3]]); | ||
| state.elements.formulaBar = document.createElement("textarea"); | ||
| state.elements.formulaBar.setAttribute("data-testid", "shouldPreserve"); | ||
|
|
||
| const savedState = createSheet([["4", "5", "6"]]); | ||
| await expectSheet(savedState.currentSheet, [[4, 5, 6]]); | ||
|
|
||
| const testFormulaCode = "functions.testFormulaCode = () => {}"; | ||
| state.load({ | ||
| sheets: savedState.sheets, | ||
| formulaCode: testFormulaCode, | ||
| }); | ||
|
|
||
| await expectSheet(state.currentSheet, [[4, 5, 6]]); | ||
| expect(state.formulaCode).toBe(testFormulaCode); | ||
| expect(state.elements.formulaBar.getAttribute("data-testid")).toBe( | ||
| "shouldPreserve", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
In my view, this is really two different tests: one tests that the bug you originally reported is fixed (i.e., the formula bar is not broken by loading a sheet), while the other tests that state.load works correctly for updating a sheet's data.
What might make sense instead is to add a much more comprehensive State.load test in addition and leave this one unchanged? (You could also break this one into two, but that seems extreme.) I'm thinking of one that has elements in cells and custom formulas and async formulas and stuff like that so we can verify that none of the tricky features are broken by the State.load implementation.
Sounds good, I'll also fix the sheet state initialization issue you caught. I ran tests since they run on git commit |
| ], | ||
| formulaCode: globals.formulaCode, | ||
| }); | ||
| save(createSaveData(globals.sheets, globals.formulaCode)); |
There was a problem hiding this comment.
Are you sure this actually works? Last I tried to move this logic elsewhere, the $effect didn't deeply detect the reactive dependencies, and the save behavior changed: saving didn't happen often enough. I don't have behavior tests for saving, did you confirm in the UI that everything behaves correctly when you do this?
Even if it does work, I'm not sure it fits in this PR. As far as I understand, changing globals to const didn't actually break anything here, and therefore doesn't necessitate this change. Since I don't have tests, I'm hesitant to add this in with everything else.
We can talk more in-person.
There was a problem hiding this comment.
You're probably right about the deeply nested reactivity breaking
2 participants
Remake of #1