Skip to content
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: check process on browser #208

Merged
merged 5 commits into from May 15, 2023
Merged

fix: check process on browser #208

merged 5 commits into from May 15, 2023

Conversation

Creskendoll
Copy link
Contributor

@Creskendoll Creskendoll commented May 4, 2023

As mentioned on #199 (review), trying to access process in a browser environment fails.

The availability of the value needs to be checked before accessing it.

You can test this easily by running process?.platform in the browser console. Instead of resolving to the expected undefined, it throws an error.

References

Relates to #199

@Creskendoll Creskendoll requested a review from a team as a code owner May 4, 2023 13:18
lib/ini.js Outdated Show resolved Hide resolved
@Creskendoll Creskendoll requested a review from wraithgar May 4, 2023 15:22
lib/ini.js Show resolved Hide resolved
@Creskendoll
Copy link
Contributor Author

Hey @wraithgar can you take another look when you have time, please? Test coverage should now pass.

@wraithgar
Copy link
Member

I can't seem to find an eslint rule that covers this, sadly. I'm kind of surprised by this and hope it's my lack of search skills. Filed npm/eslint-config#70 for looking into it later. We typically don't like to fix things like this w/o making sure it doesn't happen again (cf npm/node-semver#554)

@wraithgar wraithgar merged commit 090f64e into npm:main May 15, 2023
23 checks passed
@github-actions github-actions bot mentioned this pull request May 15, 2023
@wraithgar wraithgar self-assigned this May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants