(fix typing) typescript default export

[firsttris]

This PR fixes typescript default import.
For more details see issue #533

regards
Tristan

[gpbl]

Thanks @firsttris I will merge this, I'd like to find the time to test the typings. Since I don't know TypeScript (yet) I just trust contribution by others, however proposed changes seem to be inconsistent. Could you suggest an easy way to setup a test for the typings?

[gpbl]

I wonder then why @adidahiya added this comment :)

[firsttris]

i though about it and there a two solutions: named export and default export.

named export:

export * from "./DayPicker"

// import would be
import { DayPicker } from "./DayPicker"

default export

import { DayPicker } from "./DayPicker";
export default DayPicker;

// import would be
import DayPicker from "./DayPicker"

named export is better for refactoring, but in your case to stay consistent with JavaScript i would continue with default export

Could you suggest an easy way to setup a test for the typings?

In DefinitelyTyped they create a folder containing a tsconfig.json and the testfile.ts.
The testfile contains sample code which tests the typings. This code does not actually run, but it is type-checked when running the typescript compiler (tsc).

regards
Tristan

[PeterKottas]

You can even do both the default and named export. Imho, that's probably best way to do this. Also I would strongly suggest to export the input picker via the main lib as well. Obviously this one would be a named export.

You might have had reasons though to put it in a separate path. Maybe for code splitting? Especially considering the fact that it requires moment to run. That's a big shame tbh and I am hoping #518 will be merged soon making all this easier.

[adidahiya]

This is a breaking change, please don't make it so lightly. There is clear guidance in DefinitelyTyped's README about when to use default exports. See here: https://github.com/DefinitelyTyped/DefinitelyTyped#a-package-uses-export--but-i-prefer-to-use-default-imports-can-i-change-export--to-export-default

As @gpbl stated, there's a reason I left the comment on the line above (which is now invalidated with this code change...).

Can you point me to the code in the distributed react-day-picker package which suggests that this is the right way to export DayPicker?

[PeterKottas]

Well it's not about right way per say, it's just it doesn't work at all with typescript. These are only definitions after all. Export default is the standard way of doing it these days. I actually never seen export = something in TS before. It's not as much suggestion as both me and @firsttris encountered a problem that appears to be fixable by changing this. Reading that comment, IMHO that source file should do something like
export DayPicker from 'sourcefile' (afk)

In fact this is how I achieve similar functionality in one of my libs https://github.com/PeterKottas/guestbell-forms/blob/master/src/lib/index.ts with no problems so far. Obviously that's mostly used internally but we haven't encountered problems till now. Maybe I am missing something

[adidahiya]

it doesn't work at all with typescript

This isn't true; we use it in blueprint with the export = typings. I've used them in a handful of packages with TypeScript and they work fine. Did you look at the DefinitelyTyped README section which I linked? Was any part of that not clear enough?

Again, I am still curious to the answer to my question above:

Can you point me to the code in the distributed react-day-picker package which suggests that this is the right way to export DayPicker?

Lastly, do you acknowledge that this is a breaking change for typescript consumers, regardless of the fact that it fixes your particular tsconfig + bundling setup?

[PeterKottas]

Yeah that's cool then as long as it works for you. But the behavior we have here is inconsistent. With the documentation. Here http://react-day-picker.js.org/ it says this is the correct way of importing the lib.

import DayPicker from 'react-day-picker';

That's not the case in typescript. Therefore I wouldn't say it's fixing my particular problem. It seems to be fixing the issue where docs are inconsistent with the way it actually works.

Readme makes sense to me, however that doesn't change the fact that it doesn't work.

I agree that it's a breaking change. But in my mind, it's more of a bug fix. It's fixing something that is currently not working.

I am not sure I understand that question to be honest. @firsttris pointed out the code that is causing this problem. It's just the typings, root file for DayPicker. No sure what else are you looking for

[gpbl]

Wait @PeterKottas means till now you were using

import { DayPicker } from “react-day-picker”

?

Friends I don’t use TypeScript here, i just trust you with what you are saying 😀 And it’s sometimes difficult to understand it.

As soon as I’ve some time I’ll go into Typescript - but if someone could send a PR which is not a breaking change and with a test showing the things working right ... it would be awesome 😊

Thanks for the heads up everyone

[PeterKottas]

@gpbl Hey man, at the moment, with this

import DayPicker from 'react-day-picker';

I get:

Module '"...../node_modules/react-day-picker/types/index"' has no default export.

With

import { DayPicker } from 'react-day-picker';

I get

Module '"...../node_modules/react-day-picker/types/index"' has no exported member 'DayPicker'.
Module '"...../node_modules/react-day-picker/types/index"' resolves to a non-module entity and cannot be imported using this construct.

With

import * as DayPicker from 'react-day-picker';

I get

Module '"...../node_modules/react-day-picker/types/index"' resolves to a non-module entity and cannot 

The only way that works for me out of the box at the moment is

const DayPicker =  require('react-day-picker');

Hope it makes it clear. I am not exactly sure how many fellow typescripts are using this lib but I would assume they'll run into similar problems. That's why the breaking change might be bit too strong of a statement as as far as I can say, it's broken already.

If I go into ./node_modules/react-day-picker/types/index and manually change
export = DayPicker; to export default DayPicker;

All works as documented via import DayPicker from 'react-day-picker';

Which is what this PR does. Hope this makes all clear :)

[firsttris]

totally agree with @PeterKottas

basically there are 2 ways: named and default export.

to be consistent with the documentation, the API (the way it was implemented)
...it should work with a default import.
...best is to provide both (named and default) especially if your think about DayPickerInput

@adidahiya could it be that it works for you in blueprint because you'r using an much older version "react-day-picker": "^5.3.0"
https://github.com/palantir/blueprint/blob/master/packages/datetime/package.json#L13

with this version everything was fine on my side.

the issue occured with your PR from 30.September #487

try to upgrade to version 6.2.1 which includes your change.

looking at the typescript documentation and " export = " exsist indeed but its not considered a best practice its just to support the old traditional model

TypeScript supports export = to model the traditional CommonJS and AMD workflow. The export = syntax specifies a single object that is exported from the module. This can be a class, interface, namespace, function, or enum. When importing a module using export =, TypeScript-specific import module = require("module") must be used to import the module.

means

// export would be
import { DayPicker } from "./DayPicker";
export = DayPicker;

// import would be
import DayPicker = require("./DayPicker")

which is not consistent with the doc, and introduced a breaking change

regards
tristan

[adidahiya]

@PeterKottas @firsttris there can be many factors at play here; I don't think a simple import statement is enough to demonstrate your problem. Before you make this change, I'd like to see a minimal repro of your problem as an isolated project on Github. Until then, @gpbl, I implore you to revert this PR.

Again, this change is obviously breaking and despite the fact that it fixes an import for some build configurations, there are users relying on the old export = behavior and you should not make this change until the next major version.

looking at the typescript documentation and " export = " exsist indeed but its not considered a best practice its just to support the old traditional model

Can you link me to docs saying that it's not a best practice?

AFAIK it's a crucial bit of syntax we need while we are still using CommonJS modules on NPM (most modules are distributed this way).

Blueprint is using react-day-picker 5.5.3 (see yarn.lock, not package.json). Since there was not a breaking change to the shape of the react-day-picker export in v6.0, it doesn't matter that Blueprint is on v5.x.

I kept asking for evidence in the distributed react-day-picker package, not the documentation, that supports your export default DayPicker syntax. I see evidence to the contrary; this module.exports = statement here:

react-day-picker/DayPicker.js

Line 12 in ae71a0c

module.exports = DayPicker.default || DayPicker;

I believe what you're looking for in your build setup is a synthetic default export. Try setting "allowSyntheticDefaultExports": true in your tsconfig.json. In the meantime, please revert this change.

[firsttris]

hey adidahiya,

so you expect the user to adjust to your build configuration to get typings working?

can you provide an example project where you have 6.2.1 integrated an working?
with named or default import syntax

actually your PR broke my build.

i even tried with:
"allowSyntheticDefaultExports": true

but i still get the issue i reported here: #533
looking at the issue you see that more people are affected from this

Looking at the Typescript Modules Documentation
https://www.typescriptlang.org/docs/handbook/modules.html

you see that

export =

have to be imported like that

import module = require("module")

which I say is not best partice, because its the old, traditional way
and its no longer according to the doc's

[firsttris]

@gpbl @adidahiya
i added a small test which uses the integrated tsconfig.json to demonstrate that neither named nor default import is currently working
#542

[adidahiya]

Try using import * as DayPicker from "react-day-picker". This is how you are expected to import from packages that use module.exports = DayPicker.default || DayPicker;.

Again I think the DefinitelyTyped README gives you sufficient information to decide how to declare this package's types:

For an NPM package, export = is accurate if node -p 'require("foo")' is the export, and export default is accurate if node -p 'require("foo").default' is the export.

As for your PR #542, have you tested that the day picker import actually works at runtime? Or are you only doing a compile-time check?

[firsttris]

typings are used at compiletime, at runtime the code is transpiled javascript.
how can i run something if i can't compile it

Test with:

import * as DayPicker from "react-day-picker" // means all named exports

Typescript compiler says:

$ tsc -p types/tsconfig.json
types/test/DayPickerComponentTest.tsx(2,28): error TS2497: Module '"/home/firsttris/Projects/react-day-picker/types/index"' resolves to a non-module entity and cannot be imported using this construct.
error Command failed with exit code 1.

i dont want to be impolite but more and more it sounds to me that you are not quit sure what we are talking about.
your points sounds more like assumptions, than if you have actually tested it.

we basically need to export this classes: (TypeScript Code)
https://github.com/gpbl/react-day-picker/blob/master/types/DayPicker.d.ts#L8
https://github.com/gpbl/react-day-picker/blob/master/types/DayPickerInput.d.ts#L7

this has nothing to do with the JS Code your refercing.

[PeterKottas]

Dude @adidahiya you are either somehow misunderstanding the problem or are too proud to admit that your commit needs to be reverted. I am not exactly sure why, obviously NPM docs is not holy bible, things change get updated or removed. You got to roll with the punches. Nobody's pressing any blame, just realize that the lib currently broken! That's the important thing here.

People who update from previous version will get their build broken (like @firsttris had ) and will either have to change all imports to undocumented way which is not only difficult, but also against the docs of this library.

People who are new users like me will just fail to import it and either give up and use something else (which would be shame) or find this conversation and wonder what the f*** are we discussing here.

Any project with relatively recent version of typescript will suffer from this.

This is a great lib and I am sure our effort would be better spend than arguing about this simple one liner. For anybody who wants to reproduce this, just use https://github.com/Microsoft/TypeScript-React-Starter and import and you'll see. I've described all errors you'll get in detail in my previous comment.

Side note: I am leaving for holidays for a few weeks. I'll keep an eye on this if I can but if not, please not consider me ignoring this ;)

Lastly: If there is ANYBODY, who has this working with the latest version (6.2.1) version, please speak up. Only then am I willing to accept the fact that this is a breaking change. Otherwise, it's a bugfix.

[adidahiya]

we basically need to export this classes: (TypeScript Code)
https://github.com/gpbl/react-day-picker/blob/master/types/DayPicker.d.ts#L8
https://github.com/gpbl/react-day-picker/blob/master/types/DayPickerInput.d.ts#L7

@firsttris those are not the root export; they are submodules in the NPM package.

If you look the "main" entry from react-day-picker's package.json, it uses module.exports =, not export default: DayPicker.js. Same goes for DayPicker.dist.js.

I am not exactly sure why, obviously NPM docs is not holy bible, things change get updated or removed.

@PeterKottas Not sure what the "NPM docs" bit is referencing here, can you clarify?

For anybody who wants to reproduce this, just use https://github.com/Microsoft/TypeScript-React-Starter and import and you'll see.

Sure, and I'd be happy to be proven wrong here -- but I don't have the time to produce a minimal repro. It's on you to produce it and link us to a broken build since you are advocating for the breaking change here.

People who update from previous version will get their build broken

My PR #487 didn't change the shape of the export. There was never an export default in this package's typings. So I'm not sure how you consider this a regression. See this relevant part of the diff: https://github.com/gpbl/react-day-picker/pull/487/files#diff-88bec6beae84369c0376b56b3bb88fe1R6.

change all imports to undocumented way which is not only difficult, but also against the docs of this library

Then you might want to submit a docs PR to this library :)

[firsttris]

@adidahiya
if you look at the "types" entry from react-day-picker's package.json, it uses ./types/index.d.ts which exports the TypeScript Class:
https://github.com/gpbl/react-day-picker/blob/master/types/DayPicker.d.ts#L8

you could have actually saved the time of all your replys by simply cloning TypeScript-React-Starter and doing a simple test...

but since you do not even believe the typescript compiler... i give up on explaining to you

@gpbl
we have provided proof with:
a.) Typescript Documentation
b.) Types Test with tsc compiler & dtslint failing #542
c.) TypeScript-React-Starter Example (everyone can easily reproduce)

[adidahiya]

Alright, I was able to repro the typings regression in my project. The issue is that we no longer get the namespace declaration merging we had when all the types were in one file, index.d.ts. The solution is not to switch to a default export, but rather to make sure that the root react-day-picker export produces a module entity so that it can be imported with the import * syntax which was supported before my PR #487. I'll investigate other ways to fix this.

[gpbl]

I believe confusion comes from how we export the main module here: https://github.com/gpbl/react-day-picker/blob/master/DayPicker.js#L12-L18

This was made some versions ago to keep compatibility with CommonJS when we upgraded to babel 6. I'd like to get rid of this and export the module without specifying the defaults there. Users using CommonJS will have to specify .default manually. Would this clear up the things with TypeScript?

[firsttris]

When you import a CommonJS Module in TypeScript at development time (in vscode) only the typings are used. The be exact the typings are used by the tsc compiler. ("types" entry from Package.json.) That how you get code completion in TypeScript.
(You can easily test this in vscode or webstorm by jump into the import using CTRL + Click on the Imported module. It will jump to the typing definition rather then the javaScript code. (Main from Package.json)

While in runtime the JavaScript-Code is executed.

So i dont think a JavaScript-Code change will fix the typing.

The issue resides in the index barrel file https://github.com/gpbl/react-day-picker/blob/master/types/index.d.ts it should export DayPicker and DayPickerInput

Many famous projects just use one index.d.ts file which contains all typings...
example
react - https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/react
jest - https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/jest

Project with multiple typing files mostly use Named Export
react-router - https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react-router/index.d.ts
which can also be combined with default export

index.d.t.s example

// named
export * from "./DayPicker";
export * from "./DayPickerInput";

// default
import { DayPicker } from "./DayPicker";
export default DayPicker;

[adidahiya]

I'd like to get rid of this and export the module without specifying the defaults there. Users using CommonJS will have to specify .default manually. Would this clear up the things with TypeScript?

@gpbl yes, it would make it easier to write the typescript definitions. 👍 for this change in the next major version.

So i dont think a JavaScript-Code change will fix the typing.

@firsttris That's correct, but the important thing here is that the TS definition and the JS constructs need to match. It's possible to have completely wrong typings that type check fine, only to produce an error at runtime because the typings did not match the construct of the module you attempt to bundle in webpack. If @gpbl changes it so that node -p 'require("react-day-picker").default' is the DayPicker export as he is suggesting in his last comment, then we can change the typings to say export default DayPicker.

[gpbl]

Awesome! what’s the best thing to do now? I understand It’s OK to release a patch with this PR merged.

[firsttris]

You may understand now that its broken for TypeScript users for about one month.

For a quick fix export default DayPicker is sufficient.

Since the JavaScript package is also exporting the default i don't see the issue
https://github.com/gpbl/react-day-picker/blob/master/DayPicker.dist.js#L8
(webpack entry)

But if we think about a TypeScript user using DayPickerInput, it would be best to change your package.json - "main" to a barrel file (maybe with newer module export syntax) to also doing named & default exports similar to the way the typing barrel file would do them.

then a TypeScript user could do

import DayPicker from 'react-day-picker'; 
import * as DayPicker from 'react-day-picker';

but also

import { DayPicker } from 'react-day-picker';
import { DayPickerInput } from 'react-day-picker';

to get the correct typings.

just a suggestion, keep in mind i dont know your full build system

[adidahiya]

I understand It’s OK to release a patch with this PR merged.

Since the JavaScript package is also exporting the default i don't see the issue

No, sorry, you still can't do that... I've been trying to explain multiple times on this thread how export default DayPicker in TypeScript does not match how the module currently looks in JS. The line you linked to @firsttris is simply not a default export, by definition. module.exports = is a namespace export; if it was export default then it would be a default export.

react-day-picker/DayPicker.dist.js

Line 8 in 04e5f2a

module.exports = DayPicker.default;

[firsttris]

Like all your other assumtions... this is not correct.

module.exports = DayPicker.default; is simply CommonJs sytnax, while export default DayPicker is ES Module (Which will get transpiled by TSC or Babel).

means
export default DayPicker;
will become
module.exports = DayPicker.default;
after being transpiled by babel or tsc compiler.... (of course depends what target you set)

[adidahiya]

This is getting a little frustrating since it feels like we're talking past each other. I am well aware of the different import & export syntaxes in CommonJS / ES modules and how transpilers work.

I am pointing to a specific line in code. This line is executed when you bundle the CommonJS modules of the react-day-picker library through the "main" entry point. This line states module.exports =, not export default. The latter syntax appears in the source code of react-day-picker, not the distributed CommonJS module on NPM.

Now, of course, if you are importing from "react-day-picker/lib/src/DayPicker", that's another story -- that file does have a default export. But I assumed you were doing the standard import from the root of "react-day-picker".

[adidahiya]

You can also verify this by running node -p 'require("react-day-picker").default' in your console or testing the package in the RunKit playground. the .default export is undefined:

[firsttris]

yes because when you require('react-day-picker') you directly get what you have export - the default. so your doing default.default basically, which is undefined

you just blocking contributions because you cannot admit that you'r wrong.

even the JavaScript docs is doing an default import
http://react-day-picker.js.org/examples/

im out...

[adidahiya]

I do admit #487 was a regression (mentioned that above; I was able to repro it #539 (comment)). But this one-line change is not the right fix for all the reasons listed above.

[freehuntx]

I dont believe requiring it with default from the module can be fixing an issue from this.
Please correct me if im wrong but doesnt this make an issue with the require or import from it?
Edit: Sorry i meant export from the default import not require from it. So the default is working.

[stunaz]

I am just having the exact same issue ... @gpbl @adidahiya so what fix are you proposing? I mean really, how should we than import DayPicker ?
any workaround in the meantime while waiting for a fix?

using react-day-picker version 6.2.1 and typescript 2.6.1

[gpbl]

@stunaz first I'm adding a line to the docs :)

Anyway, we need to fix this ASAP. Since it's not clear to me what's going on, I'd just plan a major release with the right export. I'd like also to remove the common.js default exports which are the root of all the problems 😄