aichaos / rivescript-js

A RiveScript interpreter for JavaScript. RiveScript is a scripting language for chatterbots.
https://www.rivescript.com/
MIT License
377 stars 144 forks source link

[feature request] add @types/RiveScript package #318

Open MrBokeh opened 5 years ago

MrBokeh commented 5 years ago

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

kirsle commented 5 years ago

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.

kjleitz commented 5 years ago

@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!

MrBokeh commented 5 years ago

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...

kjleitz commented 5 years ago

@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?

kirsle commented 5 years ago

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.

MrBokeh commented 5 years ago

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
  },

2) That's a good idea.

3) 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

kjleitz commented 5 years ago

@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.

ArkasDev commented 4 years ago

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"]
    }
  }
}
kjleitz commented 3 years ago

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

tl;dr:

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.