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

Fix: declare var require: NodeRequire #7049

Closed
wants to merge 1 commit into from

Conversation

larsw
Copy link

@larsw larsw commented Dec 4, 2015

tsc reports an error that the require variable should be of type NodeRequire and not Require.
Works with this fix.

tsc reports an error that the require variable should be of type NodeRequire and not Require.
Works with this fix.
@dt-bot
Copy link
Member

dt-bot commented Dec 4, 2015

requirejs/require.d.ts

to author (@jbaldwin). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

@vvakame
Copy link
Member

vvakame commented Dec 10, 2015

#6083

@simonh1000
Copy link

Hope this gets merged soon

@dsebastien
Copy link
Contributor

+1 for the merge :)

@dsebastien
Copy link
Contributor

Any news about this one?

@ghost
Copy link

ghost commented Apr 7, 2016

Jumping in on this one as well.

@jbaldwin
Copy link
Contributor

jbaldwin commented Apr 8, 2016

👍 sorry for missing this one

@requiemforameme
Copy link

requiemforameme commented May 26, 2016

Any status on the PR? I know it's very minor but I don't have "rights" to merge it right now.

I'm guessing the build failed because there is some dependency on Node?

For anyone wondering how to quick fix it. Just change

Require to NodeRequire on line 398 in typings/browser/ambient/require/index.d.ts.

@vvakame
Copy link
Member

vvakame commented Jun 2, 2016

@larsw please fix travis ci failed and resolve conflict.

@mhegazy mhegazy added the Revision needed This PR needs code changes before it can be merged. label Jun 23, 2016
@mhegazy
Copy link
Contributor

mhegazy commented Jun 30, 2016

Thanks for your contribution. This PR has failing CI tests and can not be merged in at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to merge this code in, please open a new PR that has been merged and rebased with the master branch.

@mhegazy mhegazy closed this Jun 30, 2016
@dotob
Copy link
Contributor

dotob commented Jul 13, 2016

Can please make someone a suggestion how to solve this? If someone includes node and require they seem to clash. This is soo annoying.

@wgv-sethlivingston
Copy link

I "fixed" this by modifying the .d.ts files for Node and RequireJS:

declare var require: NodeRequire|Require

This needs to be done in both .d.ts files, so one PR with this fix will not fix the error. Ideas?

Blizzara added a commit to teekkarispeksi/nappikauppa2 that referenced this pull request Sep 1, 2016
- run "npm update"
- clean package.json
- switch deprecated packages to non-deprecated ones
- mainly switch from "tsd" to "typings"
NOTE: node.d.ts and require.d.ts have been manually edited to prevent
Require and NodeRequire from clashing (see e.g.
DefinitelyTyped/DefinitelyTyped#7049 (comment))

- replace "throw err; return null;" with "return Promise.reject(err)",
as typescript and throwing does not work together (see e.g.
microsoft/TypeScript#7588)
- fix some other typing stuff
- fix 'use strict';s to be on top of the files

Please run "rm -r node_modules; npm install" after getting this commit.
Blizzara added a commit to teekkarispeksi/nappikauppa2 that referenced this pull request Sep 11, 2016
- run "npm update"
- clean package.json
- switch deprecated packages to non-deprecated ones
- mainly switch from "tsd" to "typings"
NOTE: node.d.ts and require.d.ts have been manually edited to prevent
Require and NodeRequire from clashing (see e.g.
DefinitelyTyped/DefinitelyTyped#7049 (comment))

- replace "throw err; return null;" with "return Promise.reject(err)",
as typescript and throwing does not work together (see e.g.
microsoft/TypeScript#7588)
- fix some other typing stuff
- fix 'use strict';s to be on top of the files

Please run "rm -r node_modules; npm install" after getting this commit.
@Cabeda
Copy link

Cabeda commented Dec 7, 2016

+1

@pablorsk
Copy link

This issue is related with the next errors:

ERROR in node_modules/@types/webpack-env/index.d.ts
(183,13): error TS2403: Subsequent variable declarations must have the same type.  Variable 'require' must be of type 'NodeRequire', but here has type 'RequireFunction'.

ERROR in node_modules/@types/webpack-env/index.d.ts
(232,13): error TS2403: Subsequent variable declarations must have the same type.  Variable 'module' must be of type 'NodeModule', but here has type 'Module'.

ERROR in ./autoload.ts
(8,21): error TS2339: Property 'context' does not exist on type 'NodeRequire'.

Problem come when you try use this types:

  • @types/node
  • @types/webpack
  • @types/webpack-env

Any one know a provional fix?

@ghost
Copy link

ghost commented Feb 11, 2017

@pablorsk I ran into a similar error I long time ago. It was for typings modules at the time. I ended up just going to the source and changing the type of the var require to type NodeRequire from RequireFunction where the transpiler told me the error occurred. I would've thought they had that fixed by now...

@GaikwadPratik
Copy link
Contributor

@Nocomm The issue still happens with @types definitions. Are you recommending to use typings instead?

@ghost
Copy link

ghost commented Feb 19, 2017

No. The error @pablorsk was seeing was the exact same error I saw for a typings definition. There's nothing spectacularly different about the two definition libraries aside from where they are located. So, the same fix I used could possibly be applied to his problem as well.

@GaikwadPratik
Copy link
Contributor

@Dachizy okay. I had the same issue. But I did something different to resolve this. I went to source of requirejs.d.ts in @types and commented the definition of declare var require: Require; .

@zealitude
Copy link

I solve the problem by deleting the /node_module and npm install everything again.

@iProLRyan
Copy link

iProLRyan commented May 8, 2017

Hi Guys, Thanks for posting this information, it helped me produce a solution that does not involve changing type definitions from npm/typings, but rather allows us to merge interfaces and allow NodeRequire to accept require<any>('blah.css');

I have not played with requirejs, or node type definitions.

Replace your require definition with an interface, as TS will Auto merge them for us.
I placed this in /src/definitions.d.ts and it is working perfectly.

// Allow typescript to import css files (and other media)
interface NodeRequire {
    <T>(path: string): T;
    (paths: string[], callback: (...modules: any[]) => void): void;
    ensure: (paths: string[], callback: (require: <T>(path: string) => T) => void) => void;
}

If this is fundamentally wrong, please tell me why, as I am fairly new to TS. Hope it can help others.

reference: https://www.typescriptlang.org/docs/handbook/declaration-merging.html

@pablorsk
Copy link

pablorsk commented May 8, 2017

Maybe we need start again with new version of @types/node

We use "@types/node": "^7.0.15", and the problem is gone... Can you try this?

If you have the same problem, please, share your includes @types. (try with remove and reinstal node_modules fisrt)

@iProLRyan
Copy link

I may have something else going on, but I am using node ^7.0.18 - see package.json below for versions of my included types

"dependencies": {
    "@types/auth0-lock": "^10.10.1",
    "@types/jest": "^19.2.2",
    "@types/jwt-decode": "^1.4.28",
    "@types/node": "^7.0.18",
    "@types/react": "^15.0.21",
    "@types/react-bootstrap": "^0.0.49",
    "@types/react-dom": "^0.14.23",
    "@types/react-redux": "^4.4.38",
    "@types/react-router-dom": "^4.0.3",
    "@types/react-router-redux": "^5.0.0",
    "@types/redux-thunk": "^2.1.0"

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.

None yet