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

fixed rt-class to also work with lodash 4.5.0 #110

Closed
wants to merge 1 commit into from

Conversation

shaylh
Copy link

@shaylh shaylh commented Mar 10, 2016

Fixed rt-class so it won't depend on _.pick which was changed in lodash 4.5.0 and now works differently than in 3.10.0

Related issue:
#105

…sh 4.5.0 and now works differently than in 3.10.0
@shaylh
Copy link
Author

shaylh commented Mar 14, 2016

@idok , can you please review this? :)

@steezeburger
Copy link

Would love some feedback on this from Wix. Any plans on merging?

@shaylh
Copy link
Author

shaylh commented Apr 5, 2016

@idok @avi @DanShappir please help fellow react-templaters ;)

@mariawix
Copy link
Contributor

mariawix commented Apr 6, 2016

Committed in e56cd6d.
Thanks

@aDu
Copy link

aDu commented Jul 27, 2016

Thanks for the bug fix but I seem to be still experiencing a problem with using rt-class.

The problem is that "_" is not a function if I do "import * as _ from 'lodash'".

However, if I do "import _ from 'lodash'", it seems to work?

Is it because it is importing from my own node_modules/lodash and not node_modules/react-templates/node_modules/lodash?

Thanks in advance.

@nippur72
Copy link
Contributor

Do you use babel as transpiler? You should check that

import * as _ from 'lodash';

is transpiled into

var _ = require("lodash");

I don't use babel myself, but I remember people discussing about babel changing the way it does default imports.

Also do you experience the problem in rt-class only? Try doing a simple rt-repeat because it does use _.each().

@aDu
Copy link

aDu commented Jul 28, 2016

Strangely import * as _ from 'lodash' works perfectly with rt-repeat. I am really confused why.

But I have to do explicitly change it to `import _ from 'lodash' if I want it to work with rt-class.

Is it due to my installation of lodash? Perhaps import _ from 'lodash' would be the more reliable solution (unless it breaks other code)?

@nippur72
Copy link
Contributor

You don't need to import lodash in templates, as it is already imported by default (along with react).

So it must be something else. Can you please quote the rt-class code that is causing trobles?

@PierBover
Copy link

I seem to be having the same problem as @alandu50 .

I'm using Babel, Webpack, etc.

This <div rt-class="{web:true}"> produces the error:

Uncaught TypeError: _ is not a function

Not sure if it's related but I'm using this in webpack.config.js:

            {
                test: /\.rt/,
                loaders: ["babel-loader?presets[]=es2015", "react-templates-loader?modules=es6" ]
            }

@nippur72
Copy link
Contributor

nippur72 commented Aug 1, 2016

Ok, I think I've found it. The problem is in how babel transpiles the import statement.

Since it doesn't recognize lodash as an __esModule, it creates a new empty object {} and copies, one by one, the properties from loadash to the new object. What is left out is the main function _(), which by coincidence is the one used by rt-class.

You can see all that in the following transpiled code:

  • at line 3, react-templates calls _()
  • at line 11, babel turns the original lodash module into an imperfect clone
  • at line 16, babel creates an empty object instead of a "function"
  • at line 19, babel copies properties from lodash to new ob
exports.default = function () {
    return React.createElement('div', {
        'className': _({ web: true }).transform(function (res, value, key) {
            if (value) { res.push(key); }
        }, []).join(' ')
    });
};

var _lodash = require('lodash');

var _ = _interopRequireWildcard(_lodash);

function _interopRequireWildcard(obj) { 
   if (obj && obj.__esModule) { 
      return obj; 
   } else { 
      var newObj = {}; 
      if (obj != null) { 
         for (var key in obj) { 
            if (Object.prototype.hasOwnProperty.call(obj, key)) newObj[key] = obj[key]; 
         } 
      } 
      newObj.default = obj; 
      return newObj; 
   } 
}

I don't know why babel adds all that overhead. By comparison TypeScript transpiles the import into a simple require().

All that said, I think we still could fix the original issue by turning:

_({ web: true }).transform(function (res, value, key) { ... }

into

_.transform({ web: true }, function (res, value, key) { ... }

thus avoiding the call to _().

But users should be aware that they are not working with the correct lodash.

@aDu
Copy link

aDu commented Aug 3, 2016

Thanks, good spot, would it be possible for this change to be committed?

@nippur72
Copy link
Contributor

nippur72 commented Aug 3, 2016

@alandu50 I committed it in #176.

@evanpurkhiser
Copy link

@nippur72 will this be merged soon? I can't seem to come up with a work around for now.

@nippur72
Copy link
Contributor

@evanpurkhiser I'm not the maintainer so I can't say if it will be merged either. For my projects I use a fork of my own where the pending PR are all merged.

@evanpurkhiser
Copy link

Ahh sorry @nippur72, I misread your react-templates contributor tag :-)

Hopefully your fix can be committed soon, since it appears to be unusable for those using babble to transpile their templates.

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.

7 participants