Skip to content

tabulator-tables: add export statement#34756

Closed
hados99 wants to merge 3 commits intoDefinitelyTyped:masterfrom
hados99:feature/tabulator-tables-export
Closed

tabulator-tables: add export statement#34756
hados99 wants to merge 3 commits intoDefinitelyTyped:masterfrom
hados99:feature/tabulator-tables-export

Conversation

@hados99
Copy link
Copy Markdown
Contributor

@hados99 hados99 commented Apr 16, 2019

I encountered definition loading error, so I added export default statements.
and also modified tests as below

  • add import statement
  • removed unused lines
  • changed some let -> const
  • all tests are passed in my env

This pr is about #34682, but I have some git branch problem, so I made new pr

I really hope this pr to be accepted.
Thanks.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If adding a new definition:

  • The package does not already provide its own types, or cannot have its .d.ts files generated via --declaration
  • If this is for an NPM package, match the name. If not, do not conflict with the name of an NPM package.
  • Create it with dts-gen --dt, not by basing it on an existing project.
  • tslint.json should be present, and tsconfig.json should have noImplicitAny, noImplicitThis, strictNullChecks, and strictFunctionTypes set to true.

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: <>
  • Increase the version number in the header if appropriate.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

If removing a declaration:

  • If a package was never on DefinitelyTyped, you don't need to do anything. (If you wrote a package and provided types, you don't need to register it with us.)
  • Delete the package's directory.
  • Add it to notNeededPackages.json.

@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Apr 16, 2019

@hados99 Thank you for submitting this PR!

🔔 @Jojoshua - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

Copy link
Copy Markdown
Contributor

@Jojoshua Jojoshua left a comment

Choose a reason for hiding this comment

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

Adding the default export breaks existing code and should not be necessary. If you add this to your @ types then there should be no problem using it. Maybe if you had an explanation of what you couldn't get to work would be helpful.

@typescript-bot typescript-bot added Revision needed This PR needs code changes before it can be merged. and removed Awaiting reviewer feedback labels Apr 17, 2019
@typescript-bot
Copy link
Copy Markdown
Contributor

@hados99 One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you!

@hados99
Copy link
Copy Markdown
Contributor Author

hados99 commented Apr 17, 2019

Thanks for comments.

I made pr because I have problem when loading definition as below

src/index.ts:2:28 - error TS2306: File '/Users/hados/Documents/work/test/test_tabulator/node_modules/@types/tabulator-tables/index.d.ts' is not a module.

my env is typescript 3.3, 3.4 and tslint 5.13.1.

@Jojoshua
Copy link
Copy Markdown
Contributor

What does your tsconfig look like? You should have it recognize @types using "typeRoots": ["node_modules/@types"],

@hados99
Copy link
Copy Markdown
Contributor Author

hados99 commented Apr 17, 2019

I already configured typesRoot.
I installed (npm install) several type definitions

  • @types/moment
  • @types/pretty-bytes
  • @types/electron-debug
  • @types/electron-store
  • @types/jest
  • @types/lodash

but I faced loading definition error only @types/tabulator-tables

  • my tsconfig is as below (generated by vue-cli3)
{
  "compilerOptions": {
    "target": "es5",
    "module": "esnext",
    "strict": true,
    "jsx": "preserve",
    "importHelpers": true,
    "moduleResolution": "node",
    "experimentalDecorators": true,
    "esModuleInterop": true,
    "allowSyntheticDefaultImports": true,
    "strictPropertyInitialization": false,
    "sourceMap": true,
    "baseUrl": "./",
    "types": [
      "webpack-env",
      "jest"
    ],
    "paths": {
      "@/*": [
        "src/*"
      ],
      "*": ["./typings/*"]
    },
    "lib": [
      "esnext",
      "dom",
      "dom.iterable",
      "scripthost"
    ],
    "typeRoots": [
      "./typings",
      "./node_modules/@types"
    ]
  },
  "include": [
    "src/**/*.ts",
    "src/**/*.tsx",
    "src/**/*.vue",
    "tests/**/*.ts",
    "tests/**/*.tsx"
  ],
  "exclude": [
    "node_modules"
  ]
}

@Jojoshua
Copy link
Copy Markdown
Contributor

Are you trying to create a new instance of Tabulator? Try to use it like the tests do. I looked at one of the index.d.ts definitions you were using from jest and it doesn't have an export default in it either.

@hados99
Copy link
Copy Markdown
Contributor Author

hados99 commented Apr 18, 2019

  • I also read index.d.ts definitions.
    jest doesn't have export default statement,
    but index.d.ts definitions of lodash, pretty-bytes, electon-store, electron-debug has export = statements.

how about change export default to export = ?

@Jojoshua
Copy link
Copy Markdown
Contributor

I can't see adding this because it breaks existing users. The issue you are having isn't well defined. Maybe someone from the TS team can help.

@hados99
Copy link
Copy Markdown
Contributor Author

hados99 commented Apr 18, 2019

I'll find different way
Thanks for kind comments!

@hados99 hados99 closed this Apr 18, 2019
@minyoungyang
Copy link
Copy Markdown

@hados99 I have the same issue. Please let me know when you know how. Thank you.

@hados99
Copy link
Copy Markdown
Contributor Author

hados99 commented Apr 22, 2019

@minyoungyang
as a fast track, you can append export = Tabulator; to
${your workingdir}/node_modules/@types/tabulator-tables/index.d.ts

@minyoungyang
Copy link
Copy Markdown

@hados99 Thank you. I'll do it right now.

@Jojoshua
Copy link
Copy Markdown
Contributor

@hados99 @minyoung You guys might try the solution here olifolkerd/tabulator#86 (comment)

Basically add tabulator-types to your types section and then require tabulator like so...
const Tabulator = require('tabulator-tables');

This solved the issue in a vue project that was similar to yours.

"types": [ "webpack-env", "jest", "tabulator-tables" ],

@Yihao-G
Copy link
Copy Markdown

Yihao-G commented Jun 19, 2019

@Jojoshua Thanks for your contribution. However, after I imported tabulator by using const Tabulator = require('tabulator-tables'), the type of Tabulator is any and no parameter hints were given.

Here is my tsconfig:

{
    "compilerOptions": {
        "target": "es5",
        "module": "esnext",
        "moduleResolution": "node",
        "lib": [
            "esnext",
            "esnext.asynciterable",
            "dom"
        ],
        "esModuleInterop": true,
        "experimentalDecorators": true,
        "downlevelIteration": true,
        "allowJs": true,
        "sourceMap": true,
        "strict": true,
        "noImplicitAny": false,
        "noEmit": true,
        "baseUrl": ".",
        "paths": {
            "~/*": [
                "./*"
            ],
            "@/*": [
                "./*"
            ]
        },
        "types": [
            "@types/node",
            "@nuxt/vue-app",
            "tabulator-tables"
        ],
        "typeRoots": [
            "node_modules/@types"
        ]
    }
}

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

Labels

Revision needed This PR needs code changes before it can be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants