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

Add browser compatibility by making "os" dependency optional — fixes #6 #13

Closed
wants to merge 8 commits into from

Conversation

Jblew
Copy link
Contributor

@Jblew Jblew commented Jul 30, 2019

This pr simply requires "os" only in node environment. Also I've added a basic headless browser test using testcafe.

Hope you like it!

@Jblew Jblew mentioned this pull request Jul 30, 2019
@sindresorhus
Copy link
Owner

I’m happy to merge the fix, but I’m not interested in all this boilerplate for browser testing. A better fix would be to use the “browser” field in package to override the “os” dependency with an empty object.

@Jblew
Copy link
Contributor Author

Jblew commented Jul 30, 2019

Oh, sorry! I didn't check it well enough and just saw the issue. It looks like webpack is already shimming os.homedir(), so probably there is no need for this PR at all, and we can close the issue too. I do not use other bundlers so I am not sure how browserify and the others handle it. According to https://github.com/defunctzombie/package-browser-field-spec it would be safer to add browser: { os: false }. I'll create a PR for that, and I am closing this one.

PS. I am sorry to mention it here and even more sorry to bother you, but I've just added a PR to the ow repo for a functionality that I would really love to use just now for the sake of clean code in my projects ;) . It would be really great if you could merge this small change. Of course, I understand if it's not possible right now. (sindresorhus/ow#154) . Thanks!

@Jblew Jblew closed this Jul 30, 2019
@sindresorhus
Copy link
Owner

PS. I am sorry to mention it here and even more sorry to bother you, but I've just added a PR to the ow repo for a functionality that I would really love to use just now for the sake of clean code in my projects ;) . It would be really great if you could merge this small change. Of course, I understand if it's not possible right now. (sindresorhus/ow#154) . Thanks!

I saw it, but I'm on summer vacation now, so my time is limited for the next 3 weeks. I will try to look at it as soon as I can.

@Jblew
Copy link
Contributor Author

Jblew commented Jul 30, 2019

I understand, so don't spend time on it, I'll wait. And have a restful vacation! :)

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.

2 participants