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

layers support in watch mode #1503

Merged
merged 4 commits into from
Jul 29, 2023
Merged

Conversation

alixander
Copy link
Collaborator

@alixander alixander commented Jul 29, 2023

Let users develop multi-board diagrams in watch mode.

Example of clicking links to navigate in watch mode:

Kapture.2023-07-28.at.23.21.07.mp4

The back button doesn't work to update the board, at least on mac. I guess it doesn't trigger a page refresh for some reason, but this can be deferred.

closes #1176

@alixander alixander requested a review from nhooyr July 29, 2023 06:23
d2cli/watch.go Outdated Show resolved Hide resolved
// if path is "/x.svg", we just want "x"
split := strings.Split(strings.TrimPrefix(r.URL.Path, "/"), ".")
boardPath := strings.Join(split[:len(split)-1], ".")
if boardPath != w.boardPath {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of this check? Shouldn't we always render the requested board path?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

prevent duplication of compile requests on the initial load

Copy link
Contributor

Choose a reason for hiding this comment

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

Not clear to me how this accomplishes that. Can you elaborate? Also, fairly certain this is racey, I don't think you can modify w.boardPath like that without putting it under a mutex as the compilation occurs in the background.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you elaborate?

I just figure on initial load it's already compiling (behavior before this PR). So I don't want to add another recompile on top of that

fairly certain this is racey

it calls requestCompile(), which dodges races with channels.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will address in another PR if needed

Copy link
Contributor

Choose a reason for hiding this comment

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

That doesn't dodge the race. You can't modify a variable that's read in another goroutine without synchronization. The channel send merely guarantees that the compileLoop goroutine will run a compile at some point in the future, not that it isn't already running one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i did go install -race ., then reran the manual test in the PR description, and it didn't say there was a race.

Copy link
Contributor

Choose a reason for hiding this comment

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

The race detector never produces false positives, but it does produce false negatives. It can't prove your program doesn't have races, only that it does.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will look into this again later #1508

d2target/d2target.go Outdated Show resolved Hide resolved
d2target/d2target.go Outdated Show resolved Hide resolved
d2target/d2target.go Show resolved Hide resolved
@alixander alixander requested a review from nhooyr July 29, 2023 16:17
@alixander alixander marked this pull request as ready for review July 29, 2023 16:18
Copy link
Contributor

@nhooyr nhooyr left a comment

Choose a reason for hiding this comment

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

.

@alixander alixander requested a review from nhooyr July 29, 2023 20:45
@alixander alixander merged commit ccb7bc3 into terrastruct:master Jul 29, 2023
2 checks passed
@alixander alixander deleted the watch-mode-layers branch July 29, 2023 21:44
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.

svg composition for watch mode
2 participants