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

chips package relies on classnames 2.2.6 but depends on ^2.2.5 #703

Closed
kfranqueiro opened this issue Feb 28, 2019 · 4 comments
Closed

chips package relies on classnames 2.2.6 but depends on ^2.2.5 #703

kfranqueiro opened this issue Feb 28, 2019 · 4 comments
Milestone

Comments

@kfranqueiro
Copy link

This was originally reported by kittie on discord.

We are relying on the existence of a default export in classnames in ChipSet.tsx, but the default export was only added in classnames 2.2.6, whereas our package.json lists 2.2.5. If someone somehow installs 2.2.5, they will get an error like this:

index.js:590 Uncaught TypeError: classnames_1.default is not a function
    at ChipSet.get [as classes] (index.js:590)
    at Object.hasClass (index.js:603)
    at MDCChipSetFoundation.select (mdc.chips.js:3186)
    at index.js:622
    at forEachSingleChild (react.development.js:881)
    at traverseAllChildrenImpl (react.development.js:785)
    at traverseAllChildrenImpl (react.development.js:801)
    at traverseAllChildren (react.development.js:856)
    at Object.forEachChildren [as forEach] (react.development.js:901)
    at ChipSet.initChipSelection (index.js:618)

This likely flew under the radar as we have no package-lock for chips specifically, and the main package-lock is locked to 2.2.6.

@moog16 moog16 added this to the 0.11.0 milestone Mar 1, 2019
@moog16
Copy link

moog16 commented Mar 1, 2019

It occurs to me that instead of upgrading classnames package, we should the import * as classnames from 'classnames' syntax. ChipSet, as well as dialog, icon-button, snackbar, tab, tab-bar, tab-indicator/scroller were not converted correctly. I will reproduce this before making changes.

@moog16
Copy link

moog16 commented Mar 1, 2019

TLDR;

  • update all packages to ^2.2.6
  • classnames should be imported

RE classnames:
In /package.json replaced classnames: ^2.2.6 --> classnames: 2.2.5. Received error:

TS2345: Argument of type '{ 'mdc-text-field--outlined': boolean; 'mdc-text-field--textarea': boolean; 'mdc-text-field--fullwidth': boolean; 'mdc-text-field--disabled': boolean; 'mdc-text-field--with-trailing-icon': ReactElement<...> | undefined; 'mdc-text-field--with-leading-icon': ReactElement<...> | undefined; 'mdc-text-field--dense': bool...' is not assignable to parameter of type 'ClassValue'.
  Type '{ 'mdc-text-field--outlined': boolean; 'mdc-text-field--textarea': boolean; 'mdc-text-field--fullwidth': boolean; 'mdc-text-field--disabled': boolean; 'mdc-text-field--with-trailing-icon': ReactElement<...> | undefined; 'mdc-text-field--with-leading-icon': ReactElement<...> | undefined; 'mdc-text-field--dense': bool...' is not assignable to type 'undefined'.

RE importing esmodules:
references:

Since classnames@2.2.6 is exporting default, we should be importing as such:

// referenced in https://github.com/basarat/typescript-book/blob/master/docs/project/external-modules.md#default-exportsimports
import classnames from 'classnames';

In other cases where we import modules that do not use default, we should import like:

import * as classnames from 'classnames';

@dawsonc623
Copy link

dawsonc623 commented Mar 14, 2019

@moog16 You should be able to get around that issue by using esModuleInterop: true in the tsconfig.json file.

@moog16
Copy link

moog16 commented Mar 14, 2019

@dawsonc623 I agree this would work however our findings from MDC Web bloat our UMD packages. I realize we do not have importHelpers: true (like MDC Web), but could still lead to bloat with other deps/packages (just based off what @kfranqueiro found).

I eventually want to deprecate the .ts files and use ESM .js files imported from npm to match that of MDC Web. So we would pass --esModuleInterop=true there. For now I think its best to not include this.

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

No branches or pull requests

3 participants