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 unnecessary global inclusion of dom lib #92

Closed
wants to merge 1 commit into from

Conversation

AaronFriel
Copy link

TypeScript packages depending on this and importing @sindresorhus/is
end up with the lib.dom.d.ts in global scope thanks to the deleted
lines.

That's almost never what any user of the package would want, and it has
the side effect of putting a slew of common variable names (including
the variable names "event" and "name" and "parent") into a global scope.

It's particularly sinister because on many of these, TypeScript won't
complain, for example "name" is of type never.

TypeScript packages depending on this and importing `@sindresorhus/is`
end up with the lib.dom.d.ts in global scope thanks to the deleted
lines.

That's almost never what any user of the package would want, and it has
the side effect of putting a slew of common variable names (including
the variable names "event" and "name" and "parent") into a global scope.

It's particularly sinister because on many of these, TypeScript won't
complain, for example "name" is of type never.
@sindresorhus
Copy link
Owner

You have not explained why you think those lines are not needed, because they were added for a good reason.

@AaronFriel
Copy link
Author

AaronFriel commented Aug 26, 2019

This might be worthy of a TypeScript bug report, but the problem is that this library poisons the global scope with everything from lib.dom.d.ts, even in packages that don't run in a browser and thus do not have those names in scope.

So I think the right way to address #51 is that downstream users should add to their tsconfig:

{
  "compilerOptions": {
    "skipLibCheck": true
  }
}

That may be a controversial opinion, but it doesn't affect the usage of complex types in libraries, and it makes API breakage a "pay for what you use" model.

Please check out this repository for examples: https://github.com/AaronFriel/is-test

On the HEAD commit, 8a7c77c (HEAD -> master) Demonstrate original is library global name poisoning, npm run build succeeds but npm run start fails at run time:

> npm run start

> [email protected] start C:\c\af\is-test
> node ./build/src/index.js

C:\c\af\is-test\build\src\index.js:13
                ^

ReferenceError: name is not defined
    at main (C:\c\af\is-test\build\src\index.js:13:17)
    at Object.<anonymous> (C:\c\af\is-test\build\src\index.js:15:1)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:743:3)

On the initial commit, ebda52c (origin/master) Demonstrate pay-for-what-you-use skipLibCheck option and correct errors, npm run build fails, because the name is rightly not in scope.

Maybe this is why it should be a TypeScript bug report, because there probably ought to be a way to refer to the types of lib.dom.d.ts, without poisoning the global scope.

And for what it's worth, it's the suggested default of create-react-app and many other large, complex starter kits: https://github.com/facebook/create-react-app/blob/30fc0bf5ed566d9b42194d56541d278013d7928c/packages/react-scripts/scripts/utils/verifyTypeScriptSetup.js#L104

Leaving skipLibCheck on false means introducing a lot of names into the global scope that don't belong there on any package with a large node_modules tree, and it also means difficult-to-repair type errors due to strictness setting differences between the root project and dependencies, and so on.

@AaronFriel
Copy link
Author

@sindresorhus
Copy link
Owner

Maybe this is why it should be a TypeScript bug report, because there probably ought to be a way to refer to the types of lib.dom.d.ts, without poisoning the global scope.

Yes, please open a TypeScript issue about this and comment the link here.

If you open that TS issue, I'm ok with copy-pasting the DOM types we need here so we don't have to depend on lib.dom.d.ts.

So I think the right way to address #51 is that downstream users should add to their tsconfig:

I cannot assume what people have in their tsconfig. That would make the situation much worse than now.

Although maybe this is the best argument? 😄

No. That was done for performance reasons.

@AaronFriel
Copy link
Author

I've created the issue here: microsoft/TypeScript#33111

sindresorhus added a commit that referenced this pull request Sep 17, 2019
sindresorhus added a commit that referenced this pull request Oct 2, 2019
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

Successfully merging this pull request may close these issues.

2 participants