Migrate to protobuf-es #2547
Migrate to protobuf-es #2547
Conversation
|
@RumovZ Mind having a quick test of the import-csv page to confirm I've preserved your oneof handling correctly? I'll do some more testing of the various pages tomorrow. Looks like protobuf-es have landed some speedups since I last looked: bufbuild/protobuf-es#459 🎉 |
|
Tree shaking needs a bit more investigation - even without it, the congrats.js bundle has dropped from about 175k to 105k, but adding Edit: found an issue: bufbuild/protobuf-es#470. Probably easiest to just add them in as part of the build process for now. |
|
Looks good as far as I can tell! Two notes below. |
| } catch (err) { | ||
| alert(err); | ||
| throw (err); | ||
| } |
There was a problem hiding this comment.
I think the alert didn't work anyway. Was that why you've removed it? Handling this more gracefully is still on the todo list.
There was a problem hiding this comment.
The RPC methods automatically alert when an error occurs by default, and processing should stop after one of the promises fails, so I think we can probably get away without an outer catch.
(the behaviour can be disabled in the future if we want to handle it more gracefully)
There was a problem hiding this comment.
If we're talking about a popup dialog here, it doesn't work for me. If the file can't be parsed, there's nothing but an empty window.
There was a problem hiding this comment.
Do you mean on this branch? Not sure what's happening there; I was getting a pop-up in my testing. I've fixed it so instead of a generic error it relays the backend message now. Tested on Windows too to make sure it works there:
There was a problem hiding this comment.
I see, you're testing with invalid Unicode. That works here, too. I was testing with an empty file, but that seems to be a special case, because it's a frontend error.
The reason is the indexing in ImportCsvPage.svelte, line 67. But probably the dialog should abort earlier for empty files.
|
I haven't noticed any breakages in my testing today, so let's give it a try on main. If you still have issues with the error message not popping up, please let me know. |
Motivation: - Protobuf-es has a nicer API: messages are represented as classes, and fields which should exist are not marked as nullable. - As it uses modules, only the proto messages we actually use get included in our bundle output. Protobuf.js put everything in a namespace, which prevented tree-shaking, and made it awkward to access inner messages. - Code generation is sub-second instead of multiple seconds. The tradeoff is slower decoding/encoding (#2043), but that was mainly a concern for the graphs page, and was unblocked by 3715121 Approach/notes: - We generate the new protobuf-es interface in addition to existing protobuf.js interface, so we can migrate a module at a time, starting with the graphs module. - rslib:proto now generates RPC methods for TS in addition to the Python interface. The input-arg-unrolling behaviour of the Python generation is not required here, as we declare the input arg as a PlainMessage<T>, which marks it as requiring all fields to be provided. - i64 is represented as bigint in protobuf-es. We were using a patch to protobuf.js to get it to output Javascript numbers instead of long.js types, but now that our supported browser versions support bigint, it's probably worth biting the bullet and migrating to bigint use. Our IDs fit comfortably within MAX_SAFE_INTEGER, but that may not hold for future fields we add. - Oneofs are handled differently in protobuf-es, and are going to need some refactoring. Other notable changes: - Added a --mkdir arg to our build runner, so we can create a dir easily during the build on Windows. - Simplified the preference handling code, by wrapping the preferences in an outer store, instead of a separate store for each individual preference. This means a change to one preference will trigger a redraw of all components that depend on the preference store, but the redrawing is cheap after moving the data processing to Rust, and it makes the code easier to follow. - Drop async(Reactive).ts in favour of more explicit handling with await blocks/updating. - Renamed add_inputs_to_group() -> add_dependency(), and fixed it not adding dependencies to parent groups. Renamed add() -> add_action() for clarity.
+ Fix imports for multi-word proto files.
Have used caniuse.com to confirm Chromium 77, iOS 14.5 and the Chrome on Android support the full es2017-es2020 features.
To mostly maintain our old API contract, we make use of protobuf-es's ability to convert to JSON, which follows the same format as protobuf.js did. It doesn't cover all case: users who were previously changing the variant of a type will need to update their code, as assigning to a new variant no longer automatically removes the old one, which will cause an error when we try to convert back from JSON. But I suspect the large majority of users are adjusting the current variant rather than creating a new one, and this saves us having to write proxy wrappers, so it seems like a reasonable compromise. One other change I made at the same time was to rename value->kind for the oneofs in our custom study protos, as 'value' was easily confused with the 'case/value' output that protobuf-es has. With protobuf.js codegen removed, touching a proto file and invoking ./run drops from about 8s to 6s. This closes #2043.
Considerably slows down build, and not used most of the time.
Motivation:
fields which should exist are not marked as nullable.
in our bundle output. Protobuf.js put everything in a namespace, which
prevented tree-shaking, and made it awkward to access inner messages.
is slower decoding/encoding (Consider if a switch to protobuf-es is practical #2043), but that was mainly a concern for the
graphs page, and was unblocked by
3715121
Approach/notes:
protobuf.js interface, so we can migrate a module at a time, starting
with the graphs module.
interface. The input-arg-unrolling behaviour of the Python generation is
not required here, as we declare the input arg as a PlainMessage, which
marks it as requiring all fields to be provided.
protobuf.js to get it to output Javascript numbers instead of long.js
types, but now that our supported browser versions support bigint, it's
probably worth biting the bullet and migrating to bigint use. Our IDs
fit comfortably within MAX_SAFE_INTEGER, but that may not hold for future
fields we add.
some refactoring.
Other notable changes:
during the build on Windows.
in an outer store, instead of a separate store for each individual
preference. This means a change to one preference will trigger a redraw
of all components that depend on the preference store, but the redrawing
is cheap after moving the data processing to Rust, and it makes the code
easier to follow.
blocks/updating.
dependencies to parent groups. Renamed add() -> add_action() for clarity.
Once the scheduling work is done, this closes #2043