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

Added typings for v2 #277

Merged
merged 6 commits into from
Jan 17, 2019
Merged

Added typings for v2 #277

merged 6 commits into from
Jan 17, 2019

Conversation

alexandercerutti
Copy link
Contributor

This PR refers to issue #272, about the integration of typescript typings for v2.

TL;DR:
The typings work if we write at the bottom of the typings export = Bowser and import Bowser = require("bowser") in our code.

If we try with import Bowser from "bowser" and export Bowser or export default Bowser (last one as it should be), we get a false-working typings: on compiling with Webpack+Typescript, we get working typings but broken app on compilation phase.

import Bowser = require("bowser") seems to be supported only for retro-compatibility with older typescript versions and seems to be setted as deprecated. We have to investigate on this.

@coveralls
Copy link

coveralls commented Jan 8, 2019

Pull Request Test Coverage Report for Build 404

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 79.153%

Totals Coverage Status
Change from base Build 396: 0.0%
Covered Lines: 462
Relevant Lines: 479

💛 - Coveralls

index.d.ts Outdated
static map(arr: Array<any>, iterator: Function): Array<any>
}

export = Bowser;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@alexandercerutti, what if we put it like that?

export = Bowser;
export as namespace Bowser;

At least, it seems working for React: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react/index.d.ts#L45

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tomorrow I'll test on the real project, but following this post, it seems it may work: https://stackoverflow.com/questions/44847749/explain-export-and-export-as-namespace-syntax-in-typescript

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you 🙏

Copy link
Contributor Author

@alexandercerutti alexandercerutti Jan 10, 2019

Choose a reason for hiding this comment

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

I tried to convert the whole thing to React-approach like:

export = Bowser;
export as namespace Bowser;

declare namespace Bowser {
    export class Bowser { ... }
}

as to use what you said, a namespace is needed to be declared.

By doing this and import Bowser from "bowser" or import * as Bowser from "bowser", we get a module that is required to be used as "Bowser.Bowser". In fact React gets imported as import * as React, but we don't have such need, imho.
So, we can do import { Bowser } from "bowser" or look for another solution.

Copy link
Contributor Author

@alexandercerutti alexandercerutti Jan 10, 2019

Choose a reason for hiding this comment

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

EDIT: NVM, this is not valid anymore.

Okay so, I found two other ways. The key here is to no use declare module but just declare class Bowser. I also extracted the namespace and the parser class and declared a namespace called NSParser, which includes Parser class.

Then we can do like suggested by you previously or add a leading export to declare for both namespace and bowser class.

like this:

declare class Bowser { ... }
declare namespace ParserNS {
    interface ParsedResult { ... }
    // ...
    class Parser { ... }
}

If we do as you suggested, we export only Bowser.
If we set leading export, we can also export NSParser as types (the question is: might they be needed by who uses the lib?). Moreover, that might be a problem with this option, one might access directly to the Parser class. I don't think this is possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I agree that creating types is harder than it should be, with the snippet above it does seem to work in VSCode and when I build the app with webpack (ts-loader)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so I'm waiting for the approval of @lancedikson and then I'm going to do commit them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, let's commit that, I guess. I haven't tried it, but if you guys checked that and it worked, so let's have it merged. We can always release a hotfix later if something's missing.

Btw, I've found this dts-gen tool, but it doesn't seem working for me if I use it for bowser. I tried to get rid of import/export statements in order to make it be able to run through the files without babel, but it gave me just a file with declaration of Bowser class and a couple of its methods, no more.

Copy link
Contributor Author

@alexandercerutti alexandercerutti Jan 14, 2019

Choose a reason for hiding this comment

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

Perfect! Tomorrow I'm going to commit that since the full source is on my pc at work.

I never used that tool but by just reading its readme, it seems not to be that reliable to me. Even if it is from Microsoft.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'm going to push the changes.
@lancedikson, please notice that the comments for Parser.satisfies refer to a Bowser method that does not exist anymore and it still use Bowser contructor. I took all methods comments from the code.

/**
 * Check if parsed browser matches certain conditions
 *
 * @param {checkTree} checkTree It's one or two layered object,
 * which can include a platform or an OS on the first layer
 * and should have browsers specs on the bottom-laying layer
 *
 * @returns {Boolean|undefined} Whether the browser satisfies the set conditions or not.
 * Returns `undefined` when the browser is no described in the checkTree object.
 *
 * @example
 * const browser = new Bowser(UA);
 * if (browser.check({chrome: '>118.01.1322' }))
 * // or with os
 * if (browser.check({windows: { chrome: '>118.01.1322' } }))
 * // or with platforms
 * if (browser.check({desktop: { chrome: '>118.01.1322' } }))
 */

Issue #229 about npm deprecation note fix
@alexandercerutti
Copy link
Contributor Author

While I was there, I also added a "fix" for #229, which was setted as chore. 😂

@lancedikson
Copy link
Collaborator

@alexandercerutti, thanks for your work on this 👍 Merging it

@lancedikson lancedikson merged commit 5a3b509 into bowser-js:master Jan 17, 2019
@alexandercerutti
Copy link
Contributor Author

alexandercerutti commented Jan 17, 2019

@lancedikson Thanks to you! Remember to close also issue #229 😊
Anyway, oh my god, indentation fucked up 😂. Probably I mixed spaces and tabs! I hate spaces. I hope this won't break the typings.

@lancedikson
Copy link
Collaborator

Yeah, I close it as soon as it's released :)

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.

4 participants