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

Don't depend on 2 lodashes #4378

Closed
bhousel opened this issue Sep 27, 2017 · 6 comments
Closed

Don't depend on 2 lodashes #4378

bhousel opened this issue Sep 27, 2017 · 6 comments
Labels
chore-dependency Improvements to one of iD's dependencies help wanted For intermediate contributors, requires investigation or knowledge of iD code

Comments

@bhousel
Copy link
Member

bhousel commented Sep 27, 2017

followup from #4372 (comment)

We use lodash (CommonJS) in a few of our build scripts, but we now use lodash-es (ES6) in the main iD code. It seems dumb to install lodash twice, so we have 2 options:

  1. Eliminate use of lodash from the build scripts? The build scripts need to run in node 4 and up and we don't use lodash that much.
  2. Find a way to require the es6 lodash-es library from a node script. @std/esm may offer some way forward but I couldn't get it to work after playing with it for an hour or so. It's only released a few weeks ago, so we should wait and see if it improves.
@bhousel bhousel added chore-dependency Improvements to one of iD's dependencies good first issue Best for first-time contributors. No experience necessary! Hacktoberfest labels Sep 27, 2017
@Semigradsky
Copy link

  1. Add babel (for running build scripts in node 4 and using cool es2015+ features) and babel-plugin-lodash (for eliminate use paths in imports).

@bhousel? I can help if you agree to this.

@bhousel
Copy link
Member Author

bhousel commented Oct 9, 2017

Add babel (for running build scripts in node 4 and using cool es2015+ features) and babel-plugin-lodash (for eliminate use paths in imports).

I'd really prefer not to need to transpile the build scripts or introduce a babel dependency. These scripts really don't use much lodash at all, so it should be possible to replace whatever they do with traditional ES5 features.

@bhousel bhousel added help wanted For intermediate contributors, requires investigation or knowledge of iD code and removed good first issue Best for first-time contributors. No experience necessary! labels Oct 12, 2017
@DzikowskiW
Copy link
Contributor

@bhousel I worked on the issue today. I was able to set up @std/esm to work along with build scripts, which led to removing lodash dependencies from the build scripts.

However, many of the tests that run in PhantomJS depend on the original lodash (_.map, _.clone, _.isEqual, _.extend, _.reduce, _.find, _.intersection). If you look at test/index.html there is a link to lodash. How do you want to handle that? Should we have a separate file with helper functions for tests, or should we try to rewrite the tests not to use any of these functions?

@bhousel
Copy link
Member Author

bhousel commented Oct 15, 2017

@DzikowskiW good question... Those functions seem like ones we could do without.. If you submit a PR to deal with the build scripts, I'll eliminate lodash from the tests.

@bhousel bhousel added the wip Work in progress label Oct 16, 2017
@bhousel bhousel removed the wip Work in progress label Oct 18, 2017
@bhousel
Copy link
Member Author

bhousel commented Oct 18, 2017

I'll eliminate lodash from the tests.

This was done!

Going forward, no lodash in tests, plz thx. All the tests should run in all of iD's supported browsers -
you can test this by starting a local server and browsing to localhost:8080/test/

Automated testing currently runs in PhantomJS 2.1 - which is conveniently about the same level of oldness as IE11. 😭

This means:

  • No ES6
  • No Object.assign or Object.values
  • I added a polyfill to spec_helpers.js for Array.find()

@HolgerJeromin
Copy link
Contributor

Perhaps you can replace phantomjs with
https://developers.google.com/web/updates/2017/06/headless-karma-mocha-chai

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore-dependency Improvements to one of iD's dependencies help wanted For intermediate contributors, requires investigation or knowledge of iD code
Projects
None yet
Development

No branches or pull requests

4 participants