search

mathiasbynens / he

A robust HTML entity encoder/decoder written in JavaScript.
https://mths.be/he
MIT License
3.43k stars 255 forks source link

RequireJS on NodeJS (using amdefine) compatibility. #27

Closed davojan closed 9 years ago

davojan commented 9 years ago

"he" doesn't correctly exports it's API when used with RequireJS in "node-compatibility mode" as described here: http://requirejs.org/docs/node.html#nodeModules

I changed the order of environment detection logic to prevent amdefine's fake 'define' making wrong decision about AMD environment.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.67%) when pulling cbafd7388e1e07cb2ee9b3723ee53a7d56bbe3c1 on davojan:master into f16244b3fb9fb9d3a64df667982de8acf2f31f08 on mathiasbynens:master.

mathiasbynens commented 9 years ago

cc @jdalton

jdalton commented 9 years ago

A sanity check of our reasons would be good but we check for define before exports because build optimizers, like r.js add an exports object which isn't the exports we are expecting. Since he works with AMD and Node out of the box there's no need for the amdefine use.

Can you elaborate on "doesn't correctly exports"? What is it doing in the presence of amdefine?

davojan commented 9 years ago

It doesn't export anything, just empty object. When r.js is running with 'nodeRequire' configuration option it tries to load 'he' from node_modules as a regular npm module, but due to presence of 'amdefine' (it's necessary for AMD modules to work on node) 'he' thinks that it's used in AMD environment and doesn't write itself to 'module.exports'.

If you are in doubt that the opposite check order can break compatibility with some other environments, it's possible to check both conditions separately without 'else if'. I can modifly the pull-request if you'd like.

davojan commented 9 years ago

Btw lodash has the same problem and I'm going to submit the same pull-request there too :)

jdalton commented 9 years ago

It doesn't export anything, just empty object. When r.js is running with 'nodeRequire' configuration option it tries to load 'he' from node_modules as a regular npm module, but due to presence of 'amdefine' (it's necessary for AMD modules to work on node) 'he' thinks that it's used in AMD environment and doesn't write itself to 'module.exports'.

Does amdefine write to the global? If so and it's supposed to make AMD code run in Node why wouldn't he just work?

Btw lodash has the same problem and I'm going to submit the same pull-request there too :)

Hold off on that for now, I'm closer to wontfix than fix atm.

davojan commented 9 years ago

Does amdefine write to the global? If so and it's supposed to make AMD code run in Node why wouldn't he just work?

Yes it writes to the global. It doesn't work because in this mode r.js loads he from node_modules folder and treats it as a regular npm package, so it expects to see exports in this case and doesn't check 'defined' modules, I think.

May be @jrburke could clarify the issue?

we check for define before exports because build optimizers, like r.js add an exports object which isn't the exports we are expecting

My version works OK on browser-side in AMD mode (without amdefine) even with checking .exports before define.

Look, I realized that I can configure r.js to use 'he' as regular AMD module on server-side. But that is not very convenient. I'm working on as isomorphic app that has bunch of dependencies, and making them to work both on browser and server side is quite a challenge. Most of libraries doesn't have AMD compatibility in the same way as yours and I have to use separate version for browser and npm module for server.

And

If you are in doubt that the opposite check order can break compatibility with some other environments, it's possible to check both conditions separately without 'else if'. I can modifly the pull-request if you'd like.

May I try? :)

jdalton commented 9 years ago

Yes it writes to the global.

The docs don't mention that do they? Is this when it's compiled that its global or when running the code in node? It looks like for node code you have to do var define = require('amdefine')(module) in your file. How are you using amdefine? Are you using amdefine/intercept?

davojan commented 9 years ago

Ups. You've caught it! In one of those places there is a forgotten var, that's why define is written to the global. After adding the var all works fine. And lodash too.

Thanks for your help and patience.

jdalton commented 9 years ago

No prob. I'll still check in on r.js (it's been 2 yrs) and see how its compiling.