fix(workspace): Dendron will try to parse non-dendron files in onFirstOpen
#2405
Conversation
|
|
||
| // if markdown file, does it have frontmatter? | ||
| // this is a rough heuristic | ||
| return fs.readFileSync(fpath, { encoding: "utf8" }).startsWith("---"); |
There was a problem hiding this comment.
should we prefer async versions for perf reasons?
There was a problem hiding this comment.
changed it to async. something I do worry about is that its not immediately clear when functions are sync/async and we already have places in the code where we will either await a non-async function or fail to await an async function.
we should look into either:
- adding a convention (eg. isDendronNoteAsync) to be explicit about the return
- add stricter linting rules about asyncs
There was a problem hiding this comment.
added a task to oncall backlog -> [[Better Tooling around Async Functions|dendron://private/task.2022.02.14.better-tooling-around-async-functions]]
There was a problem hiding this comment.
I'm for an async convention, but additionally, how about explicitly stating its return value? isDendronNote(...): Promise<string> {} / async isDendronNote(...): string
Yeah, I'm for this - but obviously, for it to work it'll take a lot more refactoring of our existing classes. Generally I think DI libraries help once your injected objects become more complex (multi-tier injections). Where we're at today with the circ dep refactoring I haven't had to inject a multi-tiered object yet, but when we do I think it'll make sense to adopt a library such as TSyringe. |
| ); | ||
| const { onFirstOpen } = | ||
| UNSAFE_getWorkspaceWatcherPropsForTesting(ext); | ||
| expect(await onFirstOpen(editor)).toBeFalsy(); |
There was a problem hiding this comment.
minor nitpick, but this could also check that foldFrontmatter is not called since the main issue we're trying to fix is that Dendron tries to fold the frontmatter in non-note files. Or that could go into a different test if we want to keep the concerns of each test separate.
There was a problem hiding this comment.
yeah, that would be a better test. unfortunately not sure how to do this since in order to spy on a method, you need to either 1) pass in an object and the method or 2) spy on the method and inject the method
- isn't feasible because the method is private so we can't spy on the object
- isn't feasible because we don't have dependency inejction so we can't make
onFirstOpencall a spied method
This commit makes the following changes:
onFirstOpenshould only track files in the workspaceIDendronExtension(activateWatchersanddeactivate)Before, opening a big json file (eg. Dendron Logs) would cause vscode to crash because Dendron would try to parse the entire log file.
This commit also introduces two new conventions:
WorkspaceWatcheris hard to test since methods areprivateand require a working VSCode workspace to actually trigger (which we do not have access to in tests).Some solutions that were explored:
__DO_NOT_USE_IN_PROD_exposePropsForTestingmethod/convention to expose internal workspace state for testingWe went with the last option (will update in [[Testing|dendron://dendron.docs/dev.process.qa.test]] after the PR)
foo!.baror use@ts-ignore. This isn't great. In the future, we want to enable strict type checks and dis-allow type assertions.To make it easier to get to this future, this PR introduces the
UNSAFE_getWorkspaceWatcherPropsForTestingconvention: everytime you need to type cast, wrap it in a function.This groups all unsafe assertions in one place (which we can then add to a special unsafe file which we can ignore for strict type checking)
circular dependencies:
Export
Dendron Extended PR Checklist
Code
Basics
Extended
Instrumentation
Basics
Extended
Tests
Basics
Extended
NA
Docs
Basics
Basics
Close the Loop
Basics
Extended