Make Lib Browser Compatible

[maennchen]

Expose contentType to window

[maennchen]

Example of usage: https://github.com/LuvDaSun/angular-hal/blob/master/src/content-type/content-type.service.js

[dougwilson]

Use browserify, webpack, or requirejs.

[maennchen]

Since I'm supplying a library for anyone to use, I recommend using a bundler. Sad thing is that most users of angular.js are not using a bundler currently. I'd really like to use your lib. But if I can't use is without a bundler, that's a no-go for the angular-hal project.

[dougwilson]

At the very least, I cannot accept the pull request, as there are no tests for the change. I would need you to also set up web browser tests yo ensure the functionality works in browsers as changes are made.

What happens when this module adds a dependency, for example? I'm sure there are better solutions than adding window exports here, but I don't work on the front end and am not familiar with the solutions to give you a better answer. In the past for all my modules, I have never been told that those are not a solution, so I would be curious to understand how there is no possible way to solve your issue with them.

[maennchen]

I personally always use webpack. The problem is that I also provide my lib for people who are either a) downloading the js files and putting them into their source b) installing stuff via bower c) are using a bundler. (Popularity in this order.)

Currently the module is not using a dependency. As soon as it will, I will need to update my installation notes or re-evaluate if i want to use this lib. It there is interest, I could supply tests.

The only other option I see is to copy the relevant code into my own lib, which I really want to avoid.

[dougwilson]

Sure, but I think you are underestimating the request you are making here. Right now, this module only supports the engines as specified in the package.json file and nothing else. We are free to add a dependency at any time without notice or any other feature our declared engines support. I don't even code for web browsers so can very easily break stuff if this module is directly run in a web browser.

Can you please put together a comprehensive proposal for what it would take to make this module browser-compatible? Here are the rough requirements in my mind:

1. Must have a CI integration to tun unit tests in the declared web browsers.
2. Must support and run unit tests in all actively-used browsers, back to what would be similar to how we support Node.js 0.6 (IE8?).
3. Cannot include browser-only code or forks in code in what is shipped to npm.
4. Instrument with a code coverage tool in the browsers. I use Istanbul for node.js, but not sure what is available for browsers.

Have you though of using browserify to bundle a web-compatible version of this module directly in your source?

[maennchen]

I think you're overestimating this a little bit. Most of the smaller Browser JS Projects are testing their code against PhantomJS. Only the big ones like jQuery are testing further.

1. Test could be done using Karma. Karma has runners for most browsers. Browser Runners: https://www.npmjs.com/browse/keyword/karma-launcher
2. I wouldn't test all browsers, especially not IE since we couldn't run it on travis-ci. If really all browsers should be tested, it could be realized by using SauceLabs. https://saucelabs.com/opensauce/
3. https://docs.npmjs.com/misc/developers#keeping-files-out-of-your-package
4. Karma comes with istanbul. I can generate lcov files easily.

I however thought of doing this and open issues / pull requests if there are problems: http://oli.me.uk/2013/07/21/exporting-through-amd-commonjs-and-the-global-object/

[dougwilson]

When something is supported, it has to be seriously supported. Testing in phantomjs is absolutely unacceptable to me for testing web browser support, especially because you're asking someone who never writes JavaScript for a web browser to support web browsers. Without serious testing, I cannot support web browsers, period.

For item 3, I understand hoe to do that, but your pull request does not meet the requirement, as there is a browser-only fork in code that is published to npm (because you're editing index.js). Do not add browser support by modifying index.js if the link is the solution to item 3.

Your pull request even failed our current automation, so ideally the proposal would pass all current automations as well.

I would like to understand how to write JavaScript for web browsers if I am going to support them. Would it be agreeable if we could setup a meeting over Skype so you can help me learn, or wait for me to learn on my own time before I accept his kind of pull request?

[dougwilson]

I'll keep this PR open while discussion continues.

[maennchen]

I think a call via Skype could help. My nick is jonny_xail. I'm going to be available at late afternoon / evening. (16:00 - 22:00 UTC)

[dougwilson]

Cool. Those times don't for for me today, because it overlaps the end of work. Let me know what other days.

In the meantime, please feel free to update your PR such that it is fully tested, does not publish any browser code or forks to npm, passes the existing CI check it is currently failing, and allows us to add a dependency to this module.

[dougwilson]

Also, forgot one more requirement, because this became a nightmare in another module: do not check in any built/duplicated code to git (i.e. a browserified version of the module sitting next to the original version). Typically a new version would get published and since all the maintainers were server-side only people like myself, forgot there was a build step that had to be run before every commit and it would constantly be out-of-date.

[maennchen]

Those are all requirements I really would like to support. I however don't think this is possible.

[dougwilson]

/cc @wesleytodd

[jonathanong]

for browser builds, i'd rather create UMD builds for standalone usage. we can do it as a prepublish step. we shouldn't be these types of if statements in our code. i'm not sure how non-browserify/webpack would consume it though because i would rather not change the main field (i'd want the non-concatenated build unless it was babeled). a plus side is that we can run the code through babel so that it works with IE8+ (if we care about that and if we're writing with ES5/6+).

for testing, you can use browserify or whatever to create a browser bundle including the tests, then running those in a browser.

i don't mind phantomjs, but that isn't technically a browser and i've run into issues with it (might be better now). i've also run into issues with saucealbs all the time (they have like a 90% uptime).

we should probably close this and move this into a TC discussion, but i'm absolutely against writing in UMD statements like this PR and all the browser-supported modules should have the same test structure.

[wesleytodd]

@maennchen Check out https://github.com/expressjs/discussions/issues/10

Also, it should be simple to publish a module, maybe called umd-content-type, which is a wrapper around this module to expose it in a UMD format. Then your users could get the benefits and this module would not need to be updated.