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 'varify' browserify transform to support IE9,10 #61

Merged
merged 1 commit into from
Feb 5, 2015
Merged

Add 'varify' browserify transform to support IE9,10 #61

merged 1 commit into from
Feb 5, 2015

Conversation

ronjouch
Copy link
Contributor

In f4648fc I documented a way to use convict under browserify. That was fine, until trying IE<11, which currently breaks because convict uses const.

Fortunately, browserify has a feature for that, and a simple browserify transform exists for this specific const→var simplification: varify.

  • The good:
    • Fixes IE9,IE10 compatibility.
    • Only impacts browserify usage (your usage of const out of browserify persists, and unit tests impact is null).
  • The bad:
    • But additional work on convict using other recent features unsupported by IE might re-break it...
    • ... and I don't see an easy way to unit-test this...
    • ... but we can add more transforms when that happens.

Still, that improves the situation now, doesn't hurt, and is using the recommended browserify feature for this specific need, so I think it's worthwhile.

In f4648fc I documented a way to use convict under browserify. That was fine, until trying IE<11, which currently breaks because convict uses `const`.

Fortunately, browserify has [a feature for that](https://github.com/substack/node-browserify#browserifytransform), and a simple browserify transform exists for this specific const→var simplification: [varify](https://www.npmjs.com/package/varify).

* The good:
    * Fixes IE9,IE10 compatibility.
    * Only impacts browserify usage (your usage of `const` out of browserify persists, and unit tests impact is null).
* The bad:
    * But additional work on convict using other recent features unsupported by IE might re-break it...
    * ... and I don't see an easy way to unit-test this...
    * ... but we can add more transforms when that happens.

Still, that improves the situation now, doesn't hurt, and is using the recommended browserify feature for this specific need, so I think it's worthwhile.
@ronjouch
Copy link
Contributor Author

@madarche I confirm dependencies has to be used in this case. Fiddling with the package.json of my npm-cached convict, here is what npm installing my webapp results into:

a. With varify in convict's devDependencies

convict@0.6.1 node_modules/convict
├── depd@1.0.0
├── validator@3.26.0
├── optimist@0.6.1 (wordwrap@0.0.2, minimist@0.0.10)
├── moment@2.8.4
└── cjson@0.3.0 (jsonlint@1.6.0)

b. With varify in convict's dependencies

convict@0.6.1 node_modules/convict
├── depd@1.0.0
├── validator@3.26.0
├── optimist@0.6.1 (wordwrap@0.0.2, minimist@0.0.10)
├── varify@0.1.1 (through@2.3.6, redeyed@0.4.4)
├── moment@2.8.4
└── cjson@0.3.0 (jsonlint@1.6.0)

→ Because a browserify user of convict will want the transform by installing a package.json requiring it (even though s/he doesn't plan to work on it), we need to add the dep to dependencies, not devDependencies.


Now, to the concern of adding this dependency: I admit that, from a maintainers perspective, it's sad to introduce a dependency just for an (arguably small) portion of users; ideally package.json would support something like a browserDependencies, but it doesn't. The good news, though, is that the impact on node users (apart from dependency resolution) is null because the transform won't run for them.

I hope you'll see this request as a net positive (adding a minor dep, but following the major trend of making npm packages play well both in node and in the browser), but can understand you'd be reluctant to the perspective of new dependencies. Your call.

@ronjouch
Copy link
Contributor Author

ronjouch commented Feb 4, 2015

Hi. Can anyone review this? @madarche @zaach ?

@zaach
Copy link
Contributor

zaach commented Feb 5, 2015

Thanks 👍

zaach added a commit that referenced this pull request Feb 5, 2015
Add 'varify' browserify transform to support IE9,10
@zaach zaach merged commit 43f7676 into mozilla:master Feb 5, 2015
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