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

Actually include attribute-defined properties in patch computation #944

Merged
merged 4 commits into from
Aug 19, 2024

Conversation

Dekkonot
Copy link
Member

Closes #929.

It turns out the underlying issue was that we weren't accounting for ref properties at all in the patch computation. This resolves that issue by just collecting them during compute_property_patch.

Of note, I've decided to just reuse the ref_properties test files for this since it's ultimately academic either way. It adds noise to the snapshot files but I think it's fine because the only thing that really matters here is the messageCursor on the return from the read endpoint. Let me know if you'd prefer a cleaner snapshot.

@Dekkonot Dekkonot added scope: cli Relevant to the Rojo CLI skip changelog PRs that may skip the changelog enforcement check labels Jul 19, 2024
@Dekkonot Dekkonot requested a review from kennethloeffler July 19, 2024 23:08
@Dekkonot
Copy link
Member Author

And of course, like clockwork, tests start failing on MacOS.

I promise they all pass on Windows. :-)

@kennethloeffler
Copy link
Member

Code looks good, and tests pass for me on macOS, at least most of the time...

These test failures have been occurring because macOS' FSEvents API occasionally reports events that occurred before the serve session starts - i.e. in here

if source_is_file {
fs::copy(&source_path, &project_path).expect("couldn't copy project file");
} else {
fs::create_dir(&project_path).expect("Couldn't create temporary project subdirectory");
copy_recursive(&source_path, &project_path)
.expect("Couldn't copy project to temporary directory");
};
let port = get_port_number();
let port_string = port.to_string();
let rojo_process = Command::new(ROJO_PATH)
.args([
"serve",
project_path.to_str().unwrap(),
"--port",
port_string.as_str(),
])
.current_dir(working_dir)
.spawn()
.expect("Couldn't start Rojo");

we're sometimes getting events from copying the test files to temporary directories. The only public information I can find about this behavior is this Stack Overflow question: https://stackoverflow.com/questions/47679298/howto-avoid-receiving-old-events-in-fseventstream-callback-fsevents-framework-o. It seems to happen more often when the system is under higher load, but I haven't been able to make any hard correlations.

I haven't been able to find any way we can avoid this. At this point, I'm thinking we should just sleep in between copying files and starting the server, hope it's enough time for FSEvents to not report these events, and be done with it. I've done this here: #945

@Dekkonot
Copy link
Member Author

Good news: that seems to have done it. Hopefully we can find a solution that isn't that at some point, but for now I think MacOS tests taking slightly longer isn't that bad.

@Dekkonot Dekkonot requested review from kennethloeffler and removed request for kennethloeffler August 16, 2024 23:17
@Dekkonot
Copy link
Member Author

Comment: added.

@Dekkonot Dekkonot merged commit 5e1cab2 into rojo-rbx:master Aug 19, 2024
6 checks passed
@Dekkonot Dekkonot deleted the i-fixa-da-ref-props branch August 19, 2024 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: cli Relevant to the Rojo CLI skip changelog PRs that may skip the changelog enforcement check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updates for Ref properties specified with attributes are always null in /api/subscribe responses
2 participants