-
Notifications
You must be signed in to change notification settings - Fork 84
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
feat: expose detectors and separate detect and collect functions #144
Conversation
see the linked issue/pr for more information on motivation
LGTM, but it exposes |
@Le0Developer Did you really try to run Also, could you please clarify why you export
@xnerhu Not necessarily. You may state in the docs that the detector list type is |
IIRC those mostly comprise of functions related to getting the browser kind etc, which shouldn't be hard to move.
Yes, which I planned to do in a later PR, so it's easier to review.
This would require turning the |
@xnerhu That whole export block is already documented as being out of semantic versioning: Lines 49 to 50 in a153850
|
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.
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.
This may contradict an architectural decision made by @r-valitov and @xnerhu. The idea is to separate information sources from simple utility functions. I personally think the code state in the PR is good.
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.
LGTM. I can say it's a high-quality PR, thank you.
The project maintainers will make the final decision about the architecture changes in this PR.
(length === 37 && !arrayIncludes([BrowserEngineKind.Webkit, BrowserEngineKind.Gecko], browserEngine)) || | ||
(length === 39 && !arrayIncludes([BrowserKind.IE], browser)) || | ||
(length === 33 && !arrayIncludes([BrowserEngineKind.Chromium], browserEngine)) | ||
(length === 37 && !arrayIncludes([BrowserEngineKind.Webkit, BrowserEngineKind.Gecko], browserEngineKind.value)) || |
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.
Please, remove .value
from enums.
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 @Le0Developer please, merge new changes from main
@Le0Developer LGTM, thank you. |
See #143 for motivation.
Not sure if
getSources()
andgetDetectors()
should be kept, because they are no longer used.