Skip to content

Remove devtools module declaraction #1083

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

Merged

Conversation

andrewiggins
Copy link
Member

I haven't found a way to avoid getting the following error from the preact/devtools declaration in this file:

error TS2665: Invalid module name in augmentation. Module 'preact/devtools' resolves to an untyped module at 'node_modules/preact/devtools.js', which cannot be augmented.

I don't believe this declaration is really necessary though. Neither devtools.js nor debug.js have any exports so using syntax such as import * as something from 'preact/debug'; wouldn't be correct usage. TypeScript only needs the module to be declared if you try to import something from it. Since the correct usage of these modules is import 'preact/debug'; or require('preact/debug');, we don't need to declare definitions for these modules. TypeScript handles import 'preact/debug'; and require('preact/debug'); fine without any definitions.

P.S. it appears that error is because it isn't declared within a global scope (see microsoft/TypeScript#9748). However wrapping this declaration in a declare global leads to others errors. Instead of chasing that rabbit hole, I thought it made more sense to just remove it, given the rationale above.

Thoughts?

@coveralls
Copy link

coveralls commented Apr 30, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 53d3cb6 on andrewiggins:remove-devtools-decl-type into 8a54f80 on developit:master.

@valotas
Copy link
Contributor

valotas commented Apr 30, 2018

Can you also add a test for it? I believe something like

import 'preact/debug';

on top of https://github.com/developit/preact/blob/master/test/ts/preact.tsx should prove that, right?

@marvinhagemeister marvinhagemeister mentioned this pull request Apr 30, 2018
Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Verified locally with TS 2.8

@marvinhagemeister marvinhagemeister merged commit 470d173 into preactjs:master Apr 30, 2018
@andrewiggins
Copy link
Member Author

@valotas Yea, good call. I'll catch it in my next PR. Call me out if I forget :)

@andrewiggins andrewiggins deleted the remove-devtools-decl-type branch April 30, 2018 15:08
@marvinhagemeister
Copy link
Member

@andrewiggins no worries, a test for import 'preact/debug' would've been green either way, with or without the changes in this PR.

@andrewiggins
Copy link
Member Author

Oh, that's true

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.

5 participants