-
Notifications
You must be signed in to change notification settings - Fork 805
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
fix(resources): fix browser compatibility for host and os detectors #3004
Conversation
8e816e1
to
433e024
Compare
Codecov Report
@@ Coverage Diff @@
## main #3004 +/- ##
==========================================
- Coverage 92.79% 92.78% -0.01%
==========================================
Files 183 185 +2
Lines 6137 6144 +7
Branches 1301 1300 -1
==========================================
+ Hits 5695 5701 +6
- Misses 442 443 +1
|
Thanks for working on this fix. Relevant PR in contrib: |
Can you bump the patch version? When it merges I'll release it. |
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.
👍 Why making HostDetector Noop instead of omitting the export altogether?
We need to make sure our exported names are consistent in different environments so that people don't get TypeError for missing detectors. |
Is that an issue with compilation tools that produce the same bundle for browser and server environments, because if they're importing |
ESM is static by design, we cannot conditionally import or export specific names (with legit syntax). Even if we can achieve conditional exports by different entry points, I believe it would be more intuitive to stick to that principle. Further, detectors are capable of "detecting" the environmental attributes. If they cannot detect anything they recognize, they should act as a no-op and return an empty resource, like what aws-detector, gcp-detector, alibaba-cloud-detector are doing. |
Yeah.. It all makes sense, I'm just surprised that we can switch between node and browser in the builds, but not between node and nothing. I guess I'm missing something fundamental from the build step that's enabling this and/or ESM. |
Which problem is this PR solving?
Fixes #3003
Short description of the changes
Type of change
How Has This Been Tested?
Checklist: