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

Export module instead of namespace for TS declaration #751

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

sullvn
Copy link

@sullvn sullvn commented Dec 14, 2019

The source code uses modules, so now the types will too.

This fixes the issue of not being able to use the default export although it exists.

The existing workaround is to use a project-wide tsconfig.json which works around this issue (see below). It's less than ideal, as it affects the entire project of the user. This PR should resolve that.

    "esModuleInterop": true,
    "allowSyntheticDefaultImports": true,

The source code uses modules, so now the types will too.
This fixes the issue of not being able to use the default export
although it exists.
@codecov
Copy link

codecov bot commented Dec 14, 2019

Codecov Report

Merging #751 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #751   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files       154    154           
  Lines      1030   1030           
  Branches    162    162           
===================================
  Hits       1030   1030

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa0f210...fbe6972. Read the comment docs.

@iamkun
Copy link
Owner

iamkun commented Dec 20, 2019

Should this be a breaking change? It will change all the existing code from import * as dayjs from dayjs to import dayjs from dayjs I think.

@sullvn
Copy link
Author

sullvn commented Dec 26, 2019

Yep it's a breaking type-checking change -- although I'd be surprised if anyone's code with import * as dayjs from dayjs actually works with the latest emitted ES modules for Dayjs which use export default. Mine didn't, hence investigating this issue.

To elaborate, TypeScript allows you to do import package from 'package' instead of the actual import * as package from 'package'. This is done with the allowSyntheticDefaultImports flag.

There is no setting to allow you to do the opposite -- import * as package from 'package' instead of the actual import package from 'package'.

That's why I'd be surprised if anyone is actually able to use import * as dayjs from dayjs. But if they are somehow, then this would be a breaking change.

@ilisha13
Copy link

Hi! I am running into a similar issue with one of the plugins and I'm wondering if it's related.

I am trying to add the utc plugin to my dayjs in typescript.

import utc from 'dayjs/plugin/utc';
dayjs.extend(utc);

This fails with the error:

Uncaught TypeError: t is not a function
    at Function.g.extend (dayjs.min.js:1)

If I change to import * as utc from 'dayjs/plugin/utc';, it works.

@iamkun
Copy link
Owner

iamkun commented Feb 12, 2020

https://day.js.org/docs/en/installation/typescript
This might help.

And we will update this in the next major release.

@kirillgroshkov
Copy link

Yes, would be nice! Meanwhile we're forking/patching Dayjs to achieve that. Would be nice to have it by default.

Thanks for the great library!

kirillgroshkov added a commit to NaturalCycles/time-lib that referenced this pull request Apr 11, 2020
@iamkun
Copy link
Owner

iamkun commented Apr 12, 2020

is "remove "module" field as a work-around NaturalCycles/time-lib@d5f06ae" the key to this issue ?

@kirillgroshkov
Copy link

is "remove "module" field as a work-around NaturalCycles/time-lib@d5f06ae" the key to this issue ?

No, it's just a workaround.

I think there are 2 issues to fix:

  1. Make Dayjs work with both es-style imports (import dayjs from 'dayjs') and typescript-style imports (import * as dayjs from 'dayjs'). In "js world".
  2. Make Dayjs export correct types ("ts world"), so it works for both types of imports.

Right now I cannot say if this PR fixes both, or one, or none.

But if you're positive about fixing this issue (even if it becomes a Breaking change for Typescript users) - I can try to put together my version of a fix as a separate PR.

@danawoodman
Copy link

Related to #475 I believe

@sullvn
Copy link
Author

sullvn commented Jul 13, 2020

Hey everyone, to add some more context for this type of change:

The TypeScript definitions should reflect the actual JavaScript API. They should be the same; not different. That's what TS is designed for.

One of the core issues is that dayjs is implemented with a monkeypatched, object-based API, but is exported as a default value in an ES-module. It's hard to write types for this in-between API, so there's no suprise there are issues from incompatibility.

API object and monkey-patching:

const dayjs = function (date, c) {

dayjs/src/index.js

Lines 399 to 415 in 14ab808

dayjs.prototype = Dayjs.prototype
dayjs.extend = (plugin, option) => {
plugin(option, Dayjs, dayjs)
return dayjs
}
dayjs.locale = parseLocale
dayjs.isDayjs = isDayjs
dayjs.unix = timestamp => (
dayjs(timestamp * 1e3)
)
dayjs.en = Ls[L]
dayjs.Ls = Ls

And then exported as ES module default:

export default dayjs

The alternative is moving to typical ES module exports, without monkeypatching. This would play nicer with bundlers and tooling like TypeScript. Here is the above, mechanically converted:

export const extend = (plugin, option) => {
  plugin(option, Dayjs, dayjs)
  return dayjs
}

export const locale = parseLocale

export const isDayjs = isDayjs

export const unix = timestamp => (
  dayjs(timestamp * 1e3)
)

export const en = Ls[L]
export const Ls = Ls

export default dayjs

In that case, the correct TypeScript definition is trivial. It's basically the same code, but without the implementations.

Then both JS and TS would have the same ES module API that allows both styles:

//
// Usage style - 1
//
import dayjs, { isDayjs } from 'dayjs'

// Now undefined, unless monkeypatched object is still used
dayjs.isDayjs

//
// Usage style - 2
//
import * as dayjs from 'dayjs'

// Defined, even without monkeypatched object
dayjs.isDayjs

// Error, not a function
dayjs()

// Not an error, is a function
dayjs.default()

@mohamedrchalal
Copy link

This would be the correct way to export this library. Typescript explicitly follows ES standards, and as such, the "ES Module" way of doing things is the "right" way. Check out the answer to this stack overflow question https://stackoverflow.com/questions/37565709/how-to-use-namespaces-with-import-in-typescript
If this PR could be fast tracked, my colleagues and I would be eternally grateful! And also, fantastic library!

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

Successfully merging this pull request may close these issues.

6 participants