-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[core] Fix platform specific data with tests #30011
Conversation
API change check API changes are not detected in this pull request. |
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 overall! just a few nits about the assertions and a question about the changes to package.json (mostly for my own understanding)
const map = new Map<string, string>(); | ||
|
||
setPlatformSpecificData(map); | ||
assert.ok(map.has("OS")); |
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.
nit: I wonder if isNotEmpty or exists would read better as assertions here?
isNotEmpty(map.get("OS")) or exists(map.get("OS"))?
Is there a way to start using this version now? or do we have an idea when this will be released this week? |
@jeremymeng were we planning to release core packages on Thursday? |
@mcgear @xirzec The planned July release date for core is 7/11, possibly because of the 7/4 holiday. |
Packages impacted by this PR
Issues associated with this PR
Describe the problem that is addressed by this PR
Checks for undefined
process
andprocess.versions
. Added unit tests.What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
Are there test cases added in this PR? (If not, why?)
Provide a list of related PRs (if any)
Command used to generate this PR:**(Applicable only to SDK release request PRs)
Checklists