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

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

Open
MrBokeh opened this issue Jul 22, 2019 · 9 comments
Open

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

MrBokeh opened this issue Jul 22, 2019 · 9 comments

Comments

@MrBokeh
Copy link

MrBokeh commented Jul 22, 2019

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

@kirsle
Copy link
Member

kirsle commented Jul 22, 2019

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
Copy link
Contributor

kjleitz commented Aug 2, 2019

@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
Copy link
Author

MrBokeh commented Aug 3, 2019

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
Copy link
Contributor

kjleitz commented Aug 5, 2019

@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
Copy link
Member

kirsle commented Aug 6, 2019

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
Copy link
Author

MrBokeh commented Aug 6, 2019

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

@kjleitz
Copy link
Contributor

kjleitz commented Aug 7, 2019

@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
Copy link

ArkasDev commented Oct 25, 2020

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
Copy link
Contributor

kjleitz commented Dec 14, 2020

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.

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

4 participants