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

TypeScript 2.7 + esModuleInterop #2200

Merged
merged 9 commits into from
Mar 12, 2018
Merged

TypeScript 2.7 + esModuleInterop #2200

merged 9 commits into from
Mar 12, 2018

Conversation

giladgray
Copy link
Contributor

@giladgray giladgray commented Mar 4, 2018

Fixes #2144

  • verify compiled files work

Changes proposed in this pull request:

  • typescript 2.7 and enable esModuleInterop and allowSyntheticDefaultImports
  • documentalist 1.1.0 for latest typedoc with 2.7 support
  • tslib 1.9.0 for new helpers
    • ⚠️ dependency bump in all packages, 1.5 => 1.9

@giladgray giladgray requested a review from adidahiya March 4, 2018 00:54
@blueprint-bot
Copy link

fix datetime example imports

Preview: documentation | landing | table

@@ -4,7 +4,7 @@
* Licensed under the terms of the LICENSE file distributed with this project.
*/

import * as classNames from "classnames";
import classNames from "classnames";
import * as React from "react";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't we need to do the same for all import * statements? I believe namespace imports are now discouraged altogether, so we should make that change across the board in this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only needed for things that are effectively “module.exports = something” and don’t have flags marking them as originally es modules.

TS has been doing synthetic wrong. Now you’re doing it right! (Namespaces are not invocable)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems the only offenders were classnames, moment, moment-timezone

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, so we're still blocking this PR on fixing import * as moment right?

Copy link
Contributor Author

@giladgray giladgray Mar 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typescript@2.4.1:
version "2.4.1"
resolved "https://registry.yarnpkg.com/typescript/-/typescript-2.4.1.tgz#c3ccb16ddaa0b2314de031e7e6fee89e5ba346bc"
typescript@2.7.1:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you use yarn why to find out why we have this constraint? I don't want to be pinned to 2.7.1 anywhere, it has bugs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

womp. I filed TypeStrong/typedoc#720

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 they're slightly different requests

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok... given the response on my issue, I think we'll have to live with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah for now. on my issue he said he'd have something next week for 2.7.2

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey guys, I've released a patch update to TypeDoc which updates Typescript to 2.7.2

@adidahiya
Copy link
Contributor

adidahiya commented Mar 7, 2018

does this work with consumers using older TS versions? do they also have to enable the esModuleInterop to consume our typings? if they do, that's probably ok, it's a small break -- I'm sure most people are using named imports rather than import * as Blueprint from "@blueprintjs/core"

@adidahiya
Copy link
Contributor

@ericanderson IIRC you're using rollup on some projects? Mind taking a look at this PR?

@@ -4,7 +4,7 @@
* Licensed under the terms of the LICENSE file distributed with this project.
*/

import * as classNames from "classnames";
import classNames from "classnames";
import * as React from "react";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only needed for things that are effectively “module.exports = something” and don’t have flags marking them as originally es modules.

TS has been doing synthetic wrong. Now you’re doing it right! (Namespaces are not invocable)

@ericanderson
Copy link
Member

The JavaScript generated from doing this is usable by webpack, rollup, and node. What’s funny is for rollup we actually had to write plugins to convert es code from “import * as X from “Y”” to drop the star as. The es modules you were publishing before were actually wrong and are now correct

@adidahiya
Copy link
Contributor

The es modules you were publishing before were actually wrong and are now correct

yes, that's the linked issue #2144 we're trying to fix

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, you're right, I missed the moment stuff. lgtm, just fix the tslib dependency range

@@ -33,7 +33,7 @@
"@blueprintjs/core": "^2.0.0-rc.2",
"classnames": "^2.2",
"react-day-picker": "^7.0.7",
"tslib": "^1.5.0"
"tslib": "^1.9.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all of these need to be ~1.9.0, similar to the typescript version range

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think so, it's really a minimum for the new functions added to support esModuleInterop

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I thought that the behavior of these functions can change across minor versions... but that doesn't seem to be the case looking at its README. We do it for internal libraries, but this should be fine; we should probably switch over to a ^ range

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switch what over to ^ range?

makes sense to leave typescript with ~ to allow patches but exclude minors (cuz those can change things significantly)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, I meant switch our internal libraries which use "tslib": "~ over to ^. unrelated to this PR.

@adidahiya
Copy link
Contributor

I'll take this over and finish it up, hoping to publish a 2.0.0-rc.3 with this

@blueprint-bot
Copy link

semantic merge conflcit

Preview: documentation | landing | table

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.

5 participants