rtfeldman / seamless-immutable

Immutable data structures for JavaScript which are backwards-compatible with normal JS Arrays and Objects.
BSD 3-Clause "New" or "Revised" License
5.37k stars 194 forks source link

Add "browser" key to package.json #177

Closed nazar-pc closed 7 years ago

nazar-pc commented 7 years ago

This is the simplest fix for #164 possible

rtfeldman commented 7 years ago

I've never used the browser field but it seems reasonable. I assume Webpack/Browserify/etc will respect this as the default.

If so, do they permit overriding it? In other words, is someone going to have a harder time using seamless-immutable with one of those build tools if this field exists?

Also, this should definitely point to the development build. That's the safer default because it actually takes steps to enforce immutability. People should opt into the "safety features removed" prod build if they need the perf boost.

nazar-pc commented 7 years ago

I'm not sure if browser key is used/respected by any build tool (mostly because I'm not using them, however I have strong doubts that they do).

I've seen browser key in jQuery in past (not used anymore) and found it to be a good candidate for such libraries in order to differentiate builds for Node.js and for browsers.

As for development build - well, you can't point there right now (see #166).

Situation with using Bower/NPM packages in browser is awkward. There is no deterministic standard that can answer to the question "which file should be used in browser" (AFAIK).

I my framework (can't speak for anything else) there is an option for combining, minification and caching various frontend assets (this is the reason why I mostly don't use any build tools, it does everything out of the box). It tries its best to make everything completely automatically. When compression is not used, frontend is considered in "development" environment (uses browser key in current example as is), but when compression is turned on, "production" mode is assumed (and framework will try to find file with the name from browser key with .min suffix added).

With above being said I'd like to repeat suggestion from linked issue:

As a side note, would be nice to have some more common structure for production builds, like dist/seamless-immutable.js (production version) and dist/seamless-immutable.min.js (production minified version).

In context of your last comment development version might enforce immutability and production might not. However, this is no way established standard behavior and it is a bit surprising that *.min.js file differs from un-suffixed file in some way besides minification (though, might be acceptable when added to readme).

rtfeldman commented 7 years ago

As for development build - well, you can't point there right now (see #166).

166 is about the source build, not the development build. 🙂

The development build does not contain any search results for "process".

The webpack docs suggest browser would take precedence in webpack. They also mention that there is a way to override that, so I'm okay with this as long as it points to the development build instead of the production build.

nazar-pc commented 7 years ago

Nice to see webpack supports it, didn't know about this.

In my opinion something that goes into build system should be production, since artifacts will likely be used in production. While those who need development version can still manually specify what they need explicitly.

The webpack docs suggest browser would take precedence in webpack. They also mention that there is a way to override that, so I'm okay with this as long as it points to the development build instead of the production build.

webpack key might be used in addition to browser, it will have even higher precedence than browser. But I can't imagine why would anyone want development version as webpack's artifact.

rtfeldman commented 7 years ago

In my opinion something that goes into build system should be production, since artifacts will likely be used in production. While those who need development version can still manually specify what they need explicitly.

That makes sense if there's no difference between development and production, but that's not true here.

The production build improves performance by removing the safety checks that make the immutable invariants work, under the assumption that people would have caught any problems in development. That's only a reasonable assumption to make if people have been using the development build by default, which means it's important that the production build is opt-in, not the other way around.

tl;dr Making the production build the default would be dangerously irresponsible and I'm not going to do it. 🙂

nazar-pc commented 7 years ago

tl;dr Making the production build the default would be dangerously irresponsible and I'm not going to do it. 🙂

Agree, makes sense, should I update this PR now to point to development version?

rtfeldman commented 7 years ago

@nazar-pc yep, please do!

nazar-pc commented 7 years ago

Updated

rtfeldman commented 7 years ago

Thanks @nazar-pc!