Repository navigation
Add each web integration element only where the Pipeline lacks one - #105
Merged
Merged
Conversation
jwrosewell
marked this pull request as ready for review
November 14, 2026 07:51
This was referenced Nov 14, 2026
Merged
Regenerate the TypeScript types, and make every package's types compile for a TypeScript caller
#113
Merged
Draft
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
PipelineBuilder.addRequiredElementsadds the Sequence Element, the JSON Builder, the JavaScript Builder and the Set Headers Element whatever the caller already added (pipeline/fiftyone.pipeline.core/pipelineBuilder.jslines 200 to 205 at 758fea81). This has three effects.endPoint, gets a second one after it, and the Pipeline's element for the keyjavascriptbuilderis the builder's, not the caller's, because the second replaces the first in the Pipeline's map.build()stores the result back in the builder (line 315), so building twice adds every one of them twice.#53 recorded this under "Not done here". It is gap DX-node-11.
What the specification says
pipeline-specification/features/web-integration.mdlines 29 to 45 at c8128ca4:What changed
addRequiredElementscollects the keys of the elements already given, those added in parallel included, and adds each of the four only where no element has its key, comparing keys ignoring case asaccess-to-results.mdlines 10 and 11 at c8128ca4 asks of every string comparison ("String comparisons MUST be case insensitive."). So an element the caller gives underSequencecounts as the Sequence Element. A JSON Builder it adds goes immediately before a JavaScript Builder the caller added, and otherwise at the end as before. An element the caller added stays where the caller put it. With nothing added by the caller the result is the same as before, which the existing tests in the same file check.Tests
Five new tests in
pipeline/fiftyone.pipeline.core/tests/webIntegrationElements.test.js: the four elements the caller added are not added again and are the ones the Pipeline uses, a JSON Builder the Pipeline lacks goes immediately before the caller's JavaScript Builder, a Set Headers Element added in parallel counts as present and stays there, building twice adds none of them twice, and an element whose key differs only in case counts as present.Red run 43557385669, the tests on
rc's builder, where 4 failed and 497 passed.Green run 43557391894 at 4df63c5, where 501 passed.
Red for the case, the case test on the builder before it compared keys ignoring case (82a753d6), in run 43562889990, where 1 failed and 501 passed, the one failure being the case test.
Green at b6c97512, in run 43562928082, where 502 passed.
Both ran the core package's tests on one Ubuntu runner with Node.js 24 in a throwaway workflow that is not part of this draft. The first green run, 43556604078, failed one test, because the test expected the parallel Set Headers Element at the end. The builder had left it where the caller put it, which is what the page asks, so commit 4df63c5 corrects the test's expectation.
Full build
Run 43563537285 on
tmp/pr-matrix-105-b, started bytools/pr-matrix.pywithout macOS. All 7Setting 'PullRequestSha'lines give this head, b6c9751, and Configure printed "Left out 2 macOS entries". Each of the six Build and Test jobs, Ubuntu, Ubuntu ARM and Windows on Node.js 22 and 24, passed 1,388 unit tests with 2 skipped and 138 integration tests with 4 skipped, the five new tests among them. The build of the earlier head 4df63c5, run 43557677489, passed as well.macOS was left out of this draft's full build under the 9 November decision. The rc head is built with macOS before it merges into main.
What remains
Nothing in this repository. In the other languages, .NET adds each element only where none is present, comparing builder names ignoring case (
pipeline/Web/FiftyOneStartup.cslines 262 to 265 and 324 to 329 at 51degrees-dotnet 37102176), and so does Java (StartupHelpers.javalines 48 to 58 at 51degrees-java c5d4559f). Rust's draft 51degrees-rust#412 makes its Sequence Element check ignore case. Python, PHP and Go were not checked here.