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

Convert to lodash-es #1649

Closed
wants to merge 1 commit into from
Closed

Conversation

richgt
Copy link

@richgt richgt commented Jun 10, 2019

Simple conversion from lodash to lodash-es. I did as little as possible to convert, leaving code in tact wherever possible by using imports to keep variable names the same.

@samselikoff
Copy link
Collaborator

I thought this PR from @ef4 already accomplished the size reduction? Is there any precedent in other addons for lodash vs. lodash-es?

@richgt
Copy link
Author

richgt commented Jun 10, 2019

@samselikoff - To be honest, I'm not sure about size reduction. I know that without the switch to lodash-es, we're not able to use mirage in our environment because we have issues with default exports (we've had to turn them off in loader.js because of our legacy test structure). If this conflicts with other things, I understand, and can probably just end up using a fork, but thought I'd send this through for you to take a look at.

This is the conflicting code in our environment that causes issues with the standard lodash that this update fixes for me:

// Disable generation of default exports, as this would otherwise modify the
// exports objects
loader.makeDefaultExport = false;

@richgt richgt force-pushed the rglazerman/lodash-es branch from f6aefda to 168c61a Compare June 11, 2019 18:49
@richgt richgt force-pushed the rglazerman/lodash-es branch from 168c61a to c7c4053 Compare June 11, 2019 19:54
@samselikoff
Copy link
Collaborator

@ef4 I know you're busy so feel free to ignore, but wanted to see if you had any thoughts on default exports vs. named exports, and if a change like this would change the effect of #1561.

Alternatively, is there any way for us to shim the lodash default exports here so apps with makeDefaultExport = false can use Mirage? Or even something those apps themselves could do?

@ef4
Copy link
Contributor

ef4 commented Jun 18, 2019

Instead of:

-import _assign from 'lodash/assign';
+import { assign as _assign } from 'lodash-es';

Try this:

-import _assign from 'lodash/assign';
+import _assign from 'lodash-es/assign';

I think that solves your problem.

makeDefaultExport = false doesn't mean you can't use any default exports (otherwise things like import Component from '@ember/component' also wouldn't work). It just means you can only get default exports from things that are authored as real ES modules. Switching to lodash-es gives you real modules, so the default exports work.

@samselikoff
Copy link
Collaborator

Gotcha 👍

So lodash-es seems like a good & modern approach for usage in Ember addons? Should I be using ember-lodash or are there any other considerations?

@ef4
Copy link
Contributor

ef4 commented Jun 18, 2019

Ah, there is actually another gotcha now that you mention it. One that I happen to be working on fixing today.

At the moment we don't apply babel to things that are auto imported. That means that if there's any modern language features other than modules present in lodash-es, they would get served directly to browsers, which might not be what app authors want.

embroider-build/ember-auto-import#41

@samselikoff
Copy link
Collaborator

#1666 supersedes

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.

3 participants