-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add streaming extractor #9
base: master
Are you sure you want to change the base?
Conversation
@@ -5,6 +5,7 @@ | |||
"strict": true, | |||
"noImplicitAny": true, | |||
"module": "ESNext", | |||
"moduleResolution": "node", |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@@ -6,6 +6,8 @@ import { | |||
extract as meydaExtract, | |||
default as Meyda | |||
} from "meyda"; | |||
import { Observable, from, of } from "rxjs"; | |||
import { take, bufferCount, map, tap } from "rxjs/operators"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should be it's own commit, and subsequent commits should maybe have one implementation that's observable compatible. I think the observable implementation is more concise and clearer regardless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify: It might be good to remove the original implementation, and have the original two feature extractors use the observable based implementation. First would have to make the observable implementation handle hopping windows (negative and positive offsets). Maybe a separate PR, this could go in a GH issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then again, it might not, since the inline worker would now have rxjs code in it even if it's not strictly needed, and would increase the bundle size. To do: figure out how much it would increase the bundle size.
@@ -126,3 +155,31 @@ export function extractFeatureMainThread( | |||
options | |||
); | |||
} | |||
|
|||
export function extractFeatureObservable({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a separate implementation of this library that does observable? Maybe...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thinking here was that rxjs might bump the bundle size too much to be acceptable for a use case where you don't need streaming.
|
||
postMessage(await extractFeatures(buffers, features, extractionParameters)); | ||
const batchSize = internalParameters.batchSize || buffers.length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR isn't mergeable until this breaking change disappears.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've forgotten exactly why this is a breaking change, the tests pass. The tests probably aren't good enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we're in alpha, we don't care about no stinkin breaking changes!
7826709
to
639a242
Compare
639a242
to
09a33c8
Compare
No description provided.