Skip to content

[lodash] Placeholder support#24728

Merged
mhegazy merged 20 commits intoDefinitelyTyped:masterfrom
aj-r:lodash-placeholder-single-file
Apr 14, 2018
Merged

[lodash] Placeholder support#24728
mhegazy merged 20 commits intoDefinitelyTyped:masterfrom
aj-r:lodash-placeholder-single-file

Conversation

@aj-r
Copy link
Copy Markdown
Contributor

@aj-r aj-r commented Apr 5, 2018

Unfortunately, since this required adding a ridiculous number of new overloads, it caused the tests to run out of memory. To fix the tests, I needed to do the following:

  • Removed parameterless generated overloads (e.g. interface LodashPick { (): LodashPick; ... }). I don't see any use case for these overloads (they don't really do anything), so I removed them to free up some space.
  • Removed all jsdoc comments from the fp function definitions. This is unfortunate, but it made the biggest difference by far in the performance of the tests.
  • Combined FP code into one file. Now all function definitions are inside fp.d.ts, instead of being distributed among hundreds of files. Unfortunately this change makes the diff very hard to read, but it's necessary in order to make the tests pass.

So there are some drawbacks, but I think the value of placeholder support outweighs them all.

Template:

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: https://github.com/lodash/lodash/wiki/FP-Guide#placeholders
  • Increase the version number in the header if appropriate. (N/A)
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback labels Apr 5, 2018
@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Apr 5, 2018

@aj-r Thank you for submitting this PR!

🔔 @bczengel @chrootsu @stepancar @Ailrun @e-cloud @thorn0 @jtmthf @DomiR - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot typescript-bot added Author is Owner The author of this PR is a listed owner of the package. The Travis CI build failed labels Apr 5, 2018
@typescript-bot
Copy link
Copy Markdown
Contributor

@aj-r The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks!

@typescript-bot typescript-bot added the Other Approved This PR was reviewed and signed-off by a community member. label Apr 5, 2018
@aj-r
Copy link
Copy Markdown
Contributor Author

aj-r commented Apr 13, 2018

@Andy-MS This PR has been approved for 8 days. Can it be merged? The build error is lowdb, not lodash.

@mhegazy mhegazy merged commit a1ff7a2 into DefinitelyTyped:master Apr 14, 2018
@aj-r aj-r deleted the lodash-placeholder-single-file branch April 14, 2018 19:57
@maxime-agusti
Copy link
Copy Markdown

Hi guys,

Since this PR I can't include a single function from fp module :

import * as flow from "lodash/fp/flow";

When compiling I have now this issue :

error TS2497: Module '"node_modules/@types/lodash/fp/flow"' resolves to a non-module entity and cannot be imported using this construct.

Error with @types/lodash: 4.14.111, works with @types/lodash: 4.14.106.

@ghost
Copy link
Copy Markdown

ghost commented Jul 12, 2018

@maximeag You should probably be using import flow = require("lodash/fp/flow"). It's not an ES6 module, instead a function is directly the value of the commonjs module.

@antgonzales
Copy link
Copy Markdown

I realize this is an old change but does this effectively mean that lodash/fp has no type checking? I'm not able to see any errors. Example:

lodash/fp/getOr('test', 'test', 'test') // No errors
lodash/get('test', 'test') // 2345: Argument of type 'string' is not assignable to parameter of type 'Readonly<Record<string, any>>'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Author is Owner The author of this PR is a listed owner of the package. Other Approved This PR was reviewed and signed-off by a community member. Popular package This PR affects a popular package (as counted by NPM download counts).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

placeholder is missing from lodash/fp

6 participants