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

Update instructions to use default import #284

Merged
merged 2 commits into from Jan 22, 2019
Merged

Update instructions to use default import #284

merged 2 commits into from Jan 22, 2019

Conversation

JBallin
Copy link
Contributor

@JBallin JBallin commented Jan 21, 2019

No description provided.

@JBallin JBallin changed the base branch from master to production January 21, 2019 21:25
@JBallin
Copy link
Contributor Author

JBallin commented Jan 21, 2019

Similar to #236, Jest was failing for me using import * as Bowser from 'bowser' but import Bowser from 'bowser' works. Is there a downside to using the latter? Does it work for TypeScript as well?

@coveralls
Copy link

coveralls commented Jan 21, 2019

Pull Request Test Coverage Report for Build 428

  • 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 88.308%

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

💛 - Coveralls

Copy link
Collaborator

@lancedikson lancedikson left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, but I'm not sure about this asterisk for import :)

README.md Show resolved Hide resolved
@lancedikson
Copy link
Collaborator

But, it works for ES6 for sure :) I don't know why exactly we need that asterisk for TypeScript.

@JBallin
Copy link
Contributor Author

JBallin commented Jan 21, 2019

If it doesn't work for TypeScript, are you open to adding a third option for ES6?

@alexandercerutti
Copy link
Contributor

alexandercerutti commented Jan 22, 2019

@lancedikson I tried to edit the import from/to:

import * as Bowser from "bowser";
import Bowser from "bowser";

and webpack tells me tells me

TS1192: Module '"/Users/alexandercerutti/Projects/rti-lab-sdk-js/node_modules/bowser/index"' has no default export.

Then I tried to do, as you said, to change from/to

export = Bowser;
export default Bowser;

And it compiles correctly, VSCode is able to find the typings but... SURPRISE, when I try to execute in the browser it tells me:

Uncaught TypeError: Cannot read property 'getParser' of undefined.

Those are the same errors I got while testing for #277.
So for typescript (I'm compiling to "es2015"), we need to use import * as Bowser.

@lancedikson we need the asterisk because we decided to adopt the export of a namespace, which seems is the simplest way (and the unique one) to achieve this with typescript.

As I explained in #277, since Bowser is a class with static methods (so .getParser is defined in es5 as Bowser.getParser = function() { ... }). So it does not change anything if we export namespaces instead of class in this case.

Anyway, I don't know how Jest works, @JBallin. You are transpiling TS to JS and then execute the tests?

@lancedikson
Copy link
Collaborator

Thanks, @alexandercerutti. I think we're pretty ok with having three lines of declaration for Bowser: CJS, ESM and TS.

@alexandercerutti
Copy link
Contributor

alexandercerutti commented Jan 22, 2019

I've an update @lancedikson: I discovered that if we set in our tsconfig.json.compilerOptions the key

esModuleInterop: true

It will allow us to use it as import Bowser from 'bowser' and it gives problems if we use * as.
So, I think we might specify this option somehow. Are you, @JBallin, using this options in your tsconfig?

Look at CompilerOptions for --esModuleInterop

I also found that if we do this option, it does not change nothing between export = Bowser and export default Bowser.

So I think we might just write in Readme:

const Bowser = require("bowser"); // CommonJS

import * as Bowser from "bowser" // Typescript

import Bowser from "bowser" // ES6 and Typescript with --esModuleInterop enabled

@JBallin
Copy link
Contributor Author

JBallin commented Jan 22, 2019

I should’ve clarified that I’m using create-react-app which has a built-in babel preset. Sorry for the confusion, I’m NOT using TypeScript.

I think having three lines is the best solution (adding ES6). I’m not sure it’s necessary to mention esModuleInterop? Will adj PR once I get confirmation.

@alexandercerutti
Copy link
Contributor

Oh, okay. I'm using typescript + React with Webpack, so babel too.
I think it is better to mention that, else others may open new issues topics if they are using typescript and import the lib using the one for ES6 without esModuleInterop.

@JBallin
Copy link
Contributor Author

JBallin commented Jan 22, 2019

Incorporated feedback, let me know if there any other improvements that can be made.

@lancedikson lancedikson changed the base branch from production to master January 22, 2019 19:06
@lancedikson lancedikson merged commit c5d18a4 into bowser-js:master Jan 22, 2019
@JBallin JBallin deleted the readme-import-instructions branch January 22, 2019 19:22
trivikr added a commit to trivikr/aws-sdk-js-v3 that referenced this pull request Feb 2, 2021
Reason: esModuleInterop is enabled in tsconfig.
Refs: bowser-js/bowser#284
trivikr added a commit to aws/aws-sdk-js-v3 that referenced this pull request Feb 4, 2021
* fix(util-user-agent-browser): use default import from bowser

* fix: use default import for bowser

Reason: esModuleInterop is enabled in tsconfig.
Refs: bowser-js/bowser#284
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.

None yet

4 participants