Closed Le0Developer closed 7 months ago
LGTM, but it exposes detectors
publicly, so we would be obligated to keep up with backward compatibility. @r-valitov @Finesse wdyt
sending the components to the server and then running
detect()
there
@Le0Developer Did you really try to run detect
on server? As I can see, many detectors use browser APIs, which aren't available in Node.js environment. What you want to achieve requires a deeper refactoring, to make sure the detectors are pure functions (i.e. use no data other that the input arguments, and use no browser API).
Also, could you please clarify why you export detectors
? It would make sense if the exported detect
function accepted detectors as an argument. Such way you could detectors
as a base for your custom list of detectors.
but it exposes
detectors
publicly, so we would be obligated to keep up with backward compatibility
@xnerhu Not necessarily. You may state in the docs that the detector list type is Record<string, (components: ComponentDict) => DetectorResponse>
, and the actual list of functions is outside of version control.
As I can see, many detectors use browser APIs, which aren't available in Node.js environment.
IIRC those mostly comprise of functions related to getting the browser kind etc, which shouldn't be hard to move.
What you want to achieve requires a deeper refactoring
Yes, which I planned to do in a later PR, so it's easier to review.
Such way you could
detectors
as a base for your custom list of detectors
This would require turning the components
and detectors
dicts into AbtractComponentDict
and AbstractDetectorDict
respectively, which might break typing? Will need to check. Looks it won't.
but it exposes detectors publicly, so we would be obligated to keep up with backward compatibility
@xnerhu That whole export block is already documented as being out of semantic versioning:
@Le0Developer LGTM, thank you.
See #143 for motivation.
Not sure if
getSources()
andgetDetectors()
should be kept, because they are no longer used.