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

Node and requirejs typings messing up my build #2142

Closed
EricSch opened this issue Apr 19, 2018 · 18 comments
Closed

Node and requirejs typings messing up my build #2142

EricSch opened this issue Apr 19, 2018 · 18 comments

Comments

@EricSch
Copy link

EricSch commented Apr 19, 2018

Since the Node typings are moved to "dependencies" in package.json, they conflict with the requirejs typings "@types/requirejs": "2.1.31".
I understand, it isn't exactly a rollup issue, but have they to be in "dependencies" and not like before in the "devDependencies" section?
Thanks for your great work

@guybedford
Copy link
Contributor

Can you explain exactly how this type dependency is conflicting in your app? What issues are you seeing here?

@EricSch
Copy link
Author

EricSch commented Apr 20, 2018

After updating to the latetest rollup version, my build suddenly failed while compiling:

node_modules/@types/node/index.d.ts(5950,5): error TS2300: Duplicate identifier 'mod'. node_modules/@types/requirejs/index.d.ts(38,11): error TS2300: Duplicate identifier 'mod'. node_modules/@types/requirejs/index.d.ts(422,13): error TS2403: Subsequent variable declarations must have the same type. Variable 'require' must be of type 'NodeRequire', but here has type 'Require'.

I was wondering why the Node typings were installed and I found out, they where included by the rollup package.json. Without the node or requirejs typings, I don't get the error. I'm using Angular 5.0.1 with TS 2.7.2
I'm aware that has nothing do with rollup, but was introduced in our app with the change in your package.json. When you think, I should go to the typings 'maker', I totally understand 👌

@guybedford
Copy link
Contributor

@EricSch it seems like this is a particular aspect of your app that the Node types are not compatible here. I'd suggest adding a custom exclude to your tsconfig.json file to ignore the types - "exclude": ["node_modules/@types/node"] should work I think.

We do need to include the Node types as a dependency for our types to work in the first place, so removing isn't really an option unfortunately. For as amazing as TypeScript is it has never quite got JS modularity right :P

@EricSch
Copy link
Author

EricSch commented Apr 23, 2018

@guybedford Thanks for answering, will try that,
I didn't mean to remove them totally from your package, only move them back to the "devDependencies".

And I would say, Typescript has to fight with the JS modularity mess. There are too many different module systems to get everything in every case right 😉

@lukastaegert
Copy link
Member

We could also make the @types/node a peer dependency, i.e. bring your own types? Though of course then non-TypeScript users will have to deal with a nagging warning that they are missing some dependency they do no care about.

@lukastaegert
Copy link
Member

If we move them entirely to the dev dependencies, this will leave people with broken types and no information what they have to do to fix that.

@guybedford
Copy link
Contributor

Yeah peerDependencies doesn't seem to quite solve it either unfortunately. Marking as a wontfix but happy to discuss further.

@EricSch
Copy link
Author

EricSch commented Apr 27, 2018

@guybedford Excluding the node types doesn't work. TS includes automatically every typings in @types.
One solutions would be to include all my typings separately:
"compilerOptions": { "types" : ["lodash", "express"] }
But this would be cumbersome.
I still don't get it why they have to be in the dependencies? My build (via Gulp and Rollup) works fine without the node typings. So when are the typings needed? When using the Rollup API directly?

@guybedford
Copy link
Contributor

@EricSch the fact that the "node" type specifically is breaking for you in a Node workflow is a sign that the RequireJS typing approach is just plain wrong. In particular I would argue that this line - https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/requirejs/index.d.ts#L422 is invalid as it is altering an environment invariant, while RequireJS in NodeJS will not actually override the require global.

@EricSch
Copy link
Author

EricSch commented Apr 27, 2018

@guybedford I removed the requireJs typings from our package.json. They were obsolete.
But now with the latest node typings, they are messing up again:
node_modules/@types/node/index.d.ts(2381,15): error TS2300: Duplicate identifier 'URL'. node_modules/@types/node/index.d.ts(2399,15): error TS2300: Duplicate identifier 'URLSearchParams'. node_modules/@types/node/index.d.ts(2417,14): error TS2661: Cannot export 'URL'. Only local declarations can be exported from a module. node_modules/@types/node/index.d.ts(2417,19): error TS2661: Cannot export 'URLSearchParams'. Only local declarations can be exported from a module.

seems an issue with the typings.

So, those node typings are giving me only troubles. Specially annoying because I don't need them.
Couldn't you have your own typings?

@guybedford
Copy link
Contributor

@EricSch you can fix that by locking the broken Node typing to the last working version.

The hard constraint we have here is to ensure Rollup can export working typings that don't give users warnings. And leaving out the Node typing will cause warnings in VSCode and when validating types.

The only alternative I can possibly think of would be to inline our Node typings, specifically inlining the entire definition of EventEmitter in src/rollup/types.d.ts.

@kumaresan-subramani
Copy link

Hi,
i am also having same problem,
please provide any workaround for this?
DefinitelyTyped/DefinitelyTyped#31628

@bard
Copy link

bard commented Mar 5, 2019

Having node types in rollup leads to another nasty issue: it makes code compile that shouldn't.

For example, with this tsconfig.json I'm telling TypeScript that it should stick to ES5, because I'm targeting environments that don't support any further spec:

{
  "compilerOptions": {
    "target": "es5",
    "lib": ["es5"],

And indeed, if I forget from what spec find() comes from and write somewhere in my sources:

const a = ['foo', 'bar']
console.log(a.find(s => s.indexOf('oo') !== -1))

tsc helpfully complains:

$ tsc
src/index.ts:6:15 - error TS2339: Property 'find' does not exist on type 'string[]'.

6 console.log(a.find(s => s.indexOf('oo') !== -1))

Now I know I have to either get rid of it or import a polyfill.

However, if I want to bundle this project with rollup, due to @types/node being installed I get:

$ tsc
Done in 10.26s.
$

I.e. code compiles fine now and breaks at runtime.

Ideas on how I can address this?

@bard
Copy link

bard commented Mar 6, 2019

In case anyone else stumbles on this: found workaround in microsoft/TypeScript#18588 (comment) via microsoft/TypeScript#17042.

In package.json:

"postinstall": "shx rm -rf node_modules/@types/node && echo 'workaround for libs importing @types/node on browser environment'"

@shellscape
Copy link
Contributor

Hey folks. This is a saved-form message, but rest assured we mean every word. The Rollup team is attempting to clean up the Issues backlog in the hopes that the active and still-needed, still-relevant issues bubble up to the surface. With that, we're closing issues that have been open for an eon or two, and have gone stale like pirate hard-tack without activity.

We really appreciate the folks have taken the time to open and comment on this issue. Please don't confuse this closure with us not caring or dismissing your issue, feature request, discussion, or report. The issue will still be here, just in a closed state. If the issue pertains to a bug, please re-test for the bug on the latest version of Rollup and if present, please tag @shellscape and request a re-open, and we'll be happy to oblige.

@lukastaegert
Copy link
Member

Sorry for the inconvenience, Rollup@2 will finally fix this: #3395

@lukastaegert
Copy link
Member

Solved in rollup@2.0.0

@8Observer8
Copy link

8Observer8 commented Jan 29, 2021

I solved this problem by deleting "requirejs" from "tsconfig.json" > "compilerOptions" > "types" for Browserify (CommonJS) version

And I add "node" to "types":

        "types": [
            "node"
        ]

But for AMD version I added "requirejs" to "types"

package.json

{
  "name": "pick-textured-object-webgl10-ts",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1",
    "debug": "tsc -p tsconfigs/tsconfig.debug.json",
    "compile": "tsc -p tsconfigs/tsconfig.release.json",
    "bundle": "browserify public/js/main.js -o public/js/bundle.js",
    "minify": "uglifyjs public/js/bundle.js -o public/js/bundle.min.js",
    "release": "npm run compile && npm run bundle && npm run minify"
  },
  "keywords": [],
  "author": "",
  "license": "ISC",
  "dependencies": {
    "requirejs": "^2.3.6"
  },
  "devDependencies": {
    "@types/requirejs": "^2.1.32"
  }
}

tsconfigs/tsconfig.debug.json

{
    "extends": "./tsconfig.json",
    "compilerOptions": {
        "module": "AMD",
        "sourceMap": true,
        "types": [
            "requirejs",
            "gl-matrix"
        ]
    }
}

tsconfigs/tsconfig.json

{
    "compilerOptions": {
        "target": "ES5",
        "outDir": "../public/js"
    },
    "include": [
        "../src/client/**/*.ts"
    ]
}

tsconfigs/tsconfig.release.json

{
    "extends": "./tsconfig.json",
    "compilerOptions": {
        "module": "CommonJS",
        "sourceMap": false,
        "types": [
            "node"
        ]
    },
    "exclude": [
        "../src/client/RequireConfig.ts"
    ]
}

public/index.html

<!DOCTYPE html>
<html lang="en">

<head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <title>Pick Textured Object</title>

    <!-- Debug -->
    <!-- <script data-main="js/RequireConfig"
        src="https://cdnjs.cloudflare.com/ajax/libs/require.js/2.3.6/require.min.js"></script> -->
    <!-- Release -->
    <script src="js/bundle.min.js"></script>
</head>

<body>
    <canvas id="renderCanvas"></canvas>
</body>

</html>

src/client/main.ts

import { mat4 } from "gl-matrix";

function main()
{
    const matrix = mat4.create();
    console.log("Matrix:", matrix);
}

// Debug
// main();

// Release
window.onload = () => main();

src/client/RequireConfig.ts

requirejs.config({
    baseUrl: "js",
    paths: {
        "gl-matrix": "https://cdn.jsdelivr.net/npm/gl-matrix@3.3.0/gl-matrix-min"
    }
});

requirejs(["main"], () => { });

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

No branches or pull requests

7 participants