rivescript-js icon indicating copy to clipboard operation
rivescript-js copied to clipboard

[feature request] add @types/RiveScript package

Open MrBokeh opened this issue 4 years ago • 9 comments

The existing type file doesn’t work with TypeScript very well in angular, suggest to create @types/rivescript package in npm

MrBokeh avatar Jul 22 '19 01:07 MrBokeh

Hi,

I don't personally use TypeScript and the definition file was contributed by a GitHub user. If it's out of date, consider sending a pull request with an updated definition file.

The @types user on npm seems to mostly publish their type definitions from https://github.com/DefinitelyTyped/DefinitelyTyped so you could ask there about making an @types/rivescript package for npm.

kirsle avatar Jul 22 '19 22:07 kirsle

@MrBokeh What doesn't work well exactly? I use TypeScript with RiveScript, too, but I don't have any issues currently. If you describe them here and they're reproducible I can help with any necessary changes!

kjleitz avatar Aug 02 '19 13:08 kjleitz

I use TypeScript and RiveScript on the frontend with Angular with cause all sort of issues. If you use TypeScript and RiveScript with NodeJS on the backend it will work, so here are the problems:

  1. For front end (angular) there’s no fs, path etc packages (it’s for node) which need to add as empty packages in package.json as a hack
  2. for my project I don’t use webpack so I need to manually create the process.browser object so that the library will pick the browser mode as another hack
  3. the type file is out of date, so I need to import them like other old JS libs without type file... as a quick hack...

MrBokeh avatar Aug 03 '19 00:08 MrBokeh

@MrBokeh Can you post your TypeScript version and the contents of your tsconfig.json here, please? In the meantime, I'll see if I can address each of these:

  1. For front end (angular) there’s no fs, path etc packages (it’s for node) which need to add as empty packages in package.json as a hack

This one doesn't make sense to me—if you're using it on the front end it doesn't require fs or path, and I've never run into that issue. Are you maybe trying to use loadDirectory instead of loadFile? The former isn't supported in the browser, while the latter is. Another possibility is that your TS config is trying to compile node_modules/ (including the rivescript package) along with your actual project. That can be prevented with...

  "exclude": [
    "node_modules"
    // ...
  ]

...in your tsconfig.json. Just a shot in the dark.

  1. for my project I don’t use webpack so I need to manually create the process.browser object so that the library will pick the browser mode as another hack

Hm, that's a valid concern. @kirsle maybe where it looks for process.browser it could instead do something like...

if ((typeof process !== 'undefined' && process.browser) || typeof window === 'undefined') {
  return "web";
}

...since window is not available in Node. That would allow detection even in the absence of a bundling tool like webpack or browserify.

  1. the type file is out of date...

The .d.ts file works just fine for me! Can you post specific examples of it being out of date?

  1. ...so I need to import them like other old JS libs without type file...

Don't quite understand this one. Yes, if you want to use, e.g., RiveScript as a type, you have to import it (like import RiveScript from 'rivescript';) before you use it. But that's normal and I'm not sure why you'd expect the type to be accessible globally. If I'm misunderstanding your issue, could you clarify what you mean?

kjleitz avatar Aug 05 '19 13:08 kjleitz

Re: the runtime detection for node vs. web environment, the old way used to check for window and module before I switched it to look for process.browser

Something like that could be added back in as an extra fallback in case process.browser isn't set.

kirsle avatar Aug 06 '19 00:08 kirsle

Hi @kjleitz, thanks for your reply.

  1. I am 100% using "loadFile" and on the packages.json I need to do the following to avoid errors due to angular needs to use npm install the rive package instead of including it in the html file:
"browser": {
    "fs": false,
    "path": false
  },
  1. That's a good idea.

In order to do import RiveScript from "rivescript"; I need to enable the following on tsconfig:

esModuleInterop": true,
allowSyntheticDefaultImports": true,

But if the .d.ts file is working fine I don't need to do those settings just like most of the library out there. Cheers

MrBokeh avatar Aug 06 '19 08:08 MrBokeh

@MrBokeh

In order to do import RiveScript from "rivescript"; I need to enable the following on tsconfig...

I do believe you'll need to use "esModuleInterop": true ("allowSyntheticDefaultImports" should be true by default given the former, so that should be enough), but that flag is actually "highly recommended" by the TypeScript docs, so unless it breaks your build I think it's probably something you should enable.

...I need to do the following to avoid errors due to angular needs to use npm install the rive package instead of including it in the html file...

Could you post your tsconfig.json here? Without it, it's hard to diagnose the issue! I'm not sure why TS would dig into the rivescript source and determine that you need fs/path unless it's trying to compile the module (which it shouldn't, as long as you have exclude'd "node_modules"). So if you could post your tsconfig.json it would help a lot.

kjleitz avatar Aug 07 '19 01:08 kjleitz

Is there any progress on this topic? I have the same issue when I import RiveScript.

ERROR in ./node_modules/fs-readdir-recursive/index.js Module not found: Error: Can't resolve 'fs' in 'E:......\node_modules\fs-readdir-recursive' ERROR in ./node_modules/rivescript/src/rivescript.js Module not found: Error: Can't resolve 'fs' in 'E:......\node_modules\rivescript\src' ERROR in ./node_modules/fs-readdir-recursive/index.js Module not found: Error: Can't resolve 'path' in 'E:......\node_modules\fs-readdir-recursive'

Workaround: package.json

  "browser": {
    "fs": false,
    "os": false,
    "path": false
  },

tsconfig.json

{
  "files": ["src/main.ts", "src/polyfills.ts"],
  "include": ["src/**/*.d.ts"],
  "exclude": ["node_modules"],
  "compilerOptions": {
    "esModuleInterop": true,
    "outDir": "./dist/out-tsc",
    "sourceMap": true,
    "declaration": false,
    "moduleResolution": "node",
    "emitDecoratorMetadata": true,
    "noUnusedLocals": false,
    "noUnusedParameters": false,
    "allowSyntheticDefaultImports": true,
    "experimentalDecorators": true,
    "skipLibCheck": true,
    "module": "es2020",
    "target": "es5",
    "strict": false,
    "alwaysStrict": false,
    "noImplicitAny": true,
    "noImplicitThis": true,
    "types": ["node"],
    "typeRoots": ["node_modules/@types"],
    "lib": ["es6", "dom", "es2015.iterable", "es2018"],
    "baseUrl": ".",
    "paths": {
      "@src/*": ["src/*.web.ts", "src/*.ts"]
    }
  }
}

ArkasDev avatar Oct 25 '20 17:10 ArkasDev

Ah okay so @ArkasDev you may have a couple problems here. I can't say for certain, but a few things that stand out:

  • You're only include-ing .d.ts files with your "include": ["src/**/*.d.ts"] option, which is probably an issue. Try "include": ["src/**/*.ts"] instead, so that you include all .ts and .d.ts files.
  • You're specifying "types": ["node"] and "typeRoots": ["node_modules/@types"], but just to be clear, these override (and fully replace) the defaults for these options. Check out the documentation for types and typeRoots for more info. You definitely don't need the typeRoots option specified (because "node_modules/@types" is already included by default), so you should remove that since it might be replacing/overriding some other defaults. And, instead of specifying "types": ["node"], try removing that and installing @types/node instead, just in case you're narrowing your types by accident.

tl;dr:

  • Replace "include": ["src/**/*.d.ts"] with "include": ["src/**/*.ts"]
  • Remove "typeRoots": ["node_modules/@types"]
  • Remove "types": ["node"]
  • Install @types/node

Try that out and see if it works.

If it doesn't, consider moving to "strict": true. I find that it really helps with type inference across the board, and it makes your code safer, too.

kjleitz avatar Dec 14 '20 16:12 kjleitz