Skip to content

Create lodash-global for global usage #6112

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

Closed
wants to merge 1 commit into from
Closed

Create lodash-global for global usage #6112

wants to merge 1 commit into from

Conversation

myitcv
Copy link
Contributor

@myitcv myitcv commented Oct 3, 2015

Following the pattern of React, this PR creates a lodash-global.d.ts type definition.

Why is this necessary?

The following code compiles using the current type definition:

/// <reference path="../typings/lodash/lodash.d.ts" />

console.log(_.isUndefined(5));

but fails at runtime unless you have loaded lodash elsewhere. For those, like me, who use SystemJS (or other modules loaders) to load modules, we need to declare dependencies via imports, and generally rely on the compiler to tell us when we have missed one (as is the case here).

This PR creates two (linked) versions of the type definition:

  • For those people who are going to load lodash elsewhere and want to therefore take advantage of the global definitions, they should reference lodash-global.d.ts
  • For those people who use module loaders and want the above code to be a compile error because they haven't imported lodash, they should use lodash.d.ts

So if instead I reference the lodash.d.ts in this PR, the above code would give a compile error:

error TS2304: Cannot find name '_'

This forces me to import:

/// <reference path="../typings/lodash/lodash.d.ts" />

import {_} from "lodash";

console.log(_.isUndefined(5));

The transpiled output then includes the relevant module loading code to ensure lodash has been loaded before use (in my case I'm using SystemJS):

System.register(["lodash"], function(exports_1) {
    var lodash_1;
    return {
        setters:[
            function (lodash_1_1) {
                lodash_1 = lodash_1_1;
            }],
        execute: function() {
            console.log(lodash_1._.isUndefined(5));
        }
    }
});

@myitcv
Copy link
Contributor Author

myitcv commented Oct 3, 2015

The build fails when testing the following files:

knex/knex-test.ts
sequelize/sequelize-2.0.0.d.ts
sequelize/sequelize.d.ts
sequelize/sequelize-tests-2.0.0.ts
sequelize-fixtures/sequelize-fixtures-tests.ts
sequelize-fixtures/sequelize-fixtures.d.ts
sequelize/sequelize-tests.ts

but these files are unrelated to the changes in this PR.

Thoughts?

@myitcv
Copy link
Contributor Author

myitcv commented Oct 3, 2015

@bczengel @chrootsu would appreciate your thoughts on this PR.

@vvakame - your thoughts welcomed too please, especially on why the build might be failing?

@chrootsu
Copy link
Contributor

chrootsu commented Oct 3, 2015

@myitcv Very similar changes already exist #6088

@myitcv
Copy link
Contributor Author

myitcv commented Oct 4, 2015

@chrootsu - thanks for linking that request. Hopefully it's clearer from this PR what's being proposed (you can actually see the diff) and there are tests per the React model.

@chrootsu
Copy link
Contributor

chrootsu commented Oct 4, 2015

@myitcv Why did you choose the name of the internal module __Lodash rather than LoDash?
I also think that we should not create a copy of the tests to the file lodash-global-tests.ts.

@myitcv
Copy link
Contributor Author

myitcv commented Oct 5, 2015

@chrootsu - currently the ambient module and variable are both named _, so I was trying to retain that 'interface'.

As far as the naming is concerned, as I understand it this all stems from (understandably) wanting to share code between the 'module' and global definitions. This way the ambient declarations can be exported within the "lodash" module (or equivalent) and similarly imported by a different name in the global definition

If a name like Lodash were used then, assuming the lodash.d.ts from this PR with the name modified as you suggest, the following would compile:

/// <reference path="../typings/lodash/lodash.d.ts" />

let x: Lodash.MapCache;

but the following would not:

/// <reference path="../typings/lodash/lodash.d.ts" />

console.log(_.isUndefined(5));

Even though the ambient delcaration is visible via the obfuscated __Lodash, it is an attempt to introduce some consistency in the need to import declarations when using ES6 modules.

I also think that we should not create a copy of the tests to the file lodash-global-tests.ts

Agreed. I've raised this issue to discuss what can be done. The same situation arises in the React package, as I mention in that issue.

@Andael
Copy link

Andael commented Oct 5, 2015

I've closed my pull request #6088 in favor of this one, as it applies the same fix but with a better pattern (and consistently with React).

@myitcv
Copy link
Contributor Author

myitcv commented Oct 6, 2015

Updated to fix some of the failing tests.

@myitcv
Copy link
Contributor Author

myitcv commented Oct 6, 2015

Ok, now that I've actually figured out why the tests were failing before (hadn't appreciated the obvious point that type definitions/tests that reference lodash would also be running... and failing). All the tests pass

@chrootsu
Copy link
Contributor

chrootsu commented Oct 6, 2015

@myitcv I am a little confused by the name of the module begins with __. =) Can be used, LoDashTypes instead?

@Andael
Copy link

Andael commented Oct 6, 2015

@chrootsu I believe that's for consistency with React.

The __Lodash namespace is actually hidden to the consumer. If you import it with import _ = require("lodash"), then the namespace will be _. If you use lodash-global.d.ts, then the namespace will also be _.

So __Lodash is more of an internal name designed to avoid conflicts (and to remind the user that this library should be imported by one of the proper means).

@chrootsu
Copy link
Contributor

chrootsu commented Oct 6, 2015

@ander-nz Ok, I understood, thank you )

@myitcv
Copy link
Contributor Author

myitcv commented Oct 7, 2015

Related discussion: #6148

@Strate
Copy link
Contributor

Strate commented Dec 10, 2015

Is there any progress?

@myitcv
Copy link
Contributor Author

myitcv commented Dec 14, 2015

@Strate from my perspective:

  • we have stopped using lodash directly (hence we have no immediate need for this)
  • I am focussing more at this stage on the typings project and how we can use that

Hence it might make more sense for someone else to pick up this PR if it is still needed

@RyanCavanaugh
Copy link
Member

Hello and thank you for your contribution!

Due to an excessively long queue of pull requests that have become stale, we are "declaring bankruptcy" and closing all PRs opened before May 1, 2016. If you'd still like to merge this code in, please open a new PR that has been merged and rebased with the master branch.

Going forward, we are committing to review or merge all PRs on a regular basis so this bankruptcy will not occur again. We apologize for the incovenience and hope you will continue to contribute to DefinitelyTyped in the future.

Thanks
The TypeScript and DefinitelyTyped teams

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.

5 participants