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

upgrade to use lodash 4.17.2 #22

Merged
merged 1 commit into from Dec 22, 2016
Merged

upgrade to use lodash 4.17.2 #22

merged 1 commit into from Dec 22, 2016

Conversation

tchak
Copy link
Collaborator

@tchak tchak commented Mar 9, 2016


From @mike-north:
Because lodash-4 makes use of WeakMap, where available, and because in the ember ecosystem that will come by way of babel (v6 to be specific), this is blocked by a core ember-cli issue (and really an ember-cli-babel issue)

This isn't a trivial change to make, since we make broad use of babel in the ember ecosystem. If people want to help out, here are some upstream issues that are blocking progress on ember-cli (see here for the source of truth on status)

@@ -5,6 +5,9 @@ var EmberAddon = require('ember-cli/lib/broccoli/ember-addon');
module.exports = function(defaults) {
var app = new EmberAddon(defaults, {
// Add options here
babel: {
includePolyfill: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should not be needed, but right now without it there is a crash in environments without native WeakMap support (like PhantomJS) :( I am trying to figure out what's happening.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that if WeakMap is undefined, lodash-es is trying to export default undefined value, and it seems that during transpilation we getting an object with default property that contains some junk instead of undefined...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for pointing out!

Copy link

@jdalton jdalton May 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I wasn't aware of this (no one brought it up). I'll look into this.

Update:
I believe this is resolved in Babel 6. It should even work with babel-plugin-add-module-exports.

Copy link
Collaborator Author

@tchak tchak May 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdalton unfortunately ember-cli is stuck on babel 5 /cc @stefanpenner

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why am i being cc'd ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stefanpenner Sorry should have been more explicit. I rapidly mentioned to you this issue at EmberConf. The problem here is that null/undefined exports are imported as complex objects (instead of null/undefined). You said that maybe you had an idea of what the problem/fix might be.

@kellyselden
Copy link

FYI I just tried out this branch, and it worked without issue. Using
import intersectionWith from 'lodash/intersectionWith';

@tchak
Copy link
Collaborator Author

tchak commented Mar 13, 2016

@kellyselden is includePolyfill set to true in your projects ember-cli-build.js?

@kellyselden
Copy link

@tchak Nope. No babel options.

@tchak
Copy link
Collaborator Author

tchak commented Mar 14, 2016

@kellyselden Two reasons it works for you I think. First, you using individual import. So some parts of lodash are not executed. Second, if you trying on half recent chrome or firefox, they probably support WeakMap, Map and Set. When I test with recent browsers, test only fails in PhantomJS. But I definitely do not want for the addon to surprisingly crash on older browsers...

@urbany
Copy link

urbany commented Mar 30, 2016

Any update on this?

@Blackening999
Copy link

Updates????

@musaffa
Copy link

musaffa commented Jun 23, 2016

@tchak Any update?

@Rastopyr Rastopyr mentioned this pull request Jul 16, 2016
@devinus
Copy link

devinus commented Aug 18, 2016

Why does lodash require WeakMap support?

@jdalton
Copy link

jdalton commented Aug 18, 2016

Lodash uses WeakMap when it's available to store function wrapper metadata.
The issue here is ember-lodash is stuck on an older buggier Babel version.

@tchak
Copy link
Collaborator Author

tchak commented Aug 19, 2016

I must say, I ended up using embor-browserify for lodash on my current project given that even without this problem there is no lodash/fp es build, and I am only using lodash/fp...

@mike-north
Copy link
Owner

Because lodash-4 makes use of WeakMap, where available, and because in the ember ecosystem that will come by way of babel (v6 to be specific), this is blocked by a core ember-cli issue (and really an ember-cli-babel issue)

This isn't a trivial change to make, since we make broad use of babel in the ember ecosystem. If people want to help out, here are some upstream issues that are blocking progress on ember-cli (see here for the source of truth on status)

@stefanpenner
Copy link
Contributor

@jdalton both sides have bugs. v5 and v6 :( Its tricky to balance those trade-offs.

@stefanpenner
Copy link
Contributor

stefanpenner commented Aug 19, 2016

but remember as long as your addon produces ES5 safe deanonymized AMD we don't care how you go there, if babel 6 works in your addon, it is fine to use.

@josemarluedke
Copy link
Contributor

Is there anything we can do to get this rolling again? I guess I don't understand the issue here with babel 6, but is there any other alternative to make this upgrade?

@maxkoryukov maxkoryukov mentioned this pull request Dec 4, 2016
@tchak tchak changed the title upgrade to use lodash 4.6.1 upgrade to use lodash 4.17.2 Dec 22, 2016
@tchak
Copy link
Collaborator Author

tchak commented Dec 22, 2016

It works! I found a way!

Exports of undefined values are converted to some random objects. So I replace undefined with null in _getNative method.

@mike-north should we do as planed a 3.0 and a 4.0 releases?

@tchak tchak merged commit 354f24a into master Dec 22, 2016
@tchak tchak mentioned this pull request Dec 22, 2016
@tchak tchak deleted the lodash-es-4.6.1 branch December 22, 2016 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet