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

Add file extensions to imports #294

Merged
merged 1 commit into from
Mar 2, 2019

Conversation

FostUK
Copy link
Contributor

@FostUK FostUK commented Feb 14, 2019

In order to support loading Bowser directly in the browser without transpilation this PR :

  • Adds .js file extensions to imports.
  • Adds ES6 style export to utils and changes related utils imports
  • Updates eslint rule for import extensions

Unit tests appear to need no changes as functionality is the same.

Some thoughts
As utils.js is a class collection of static functions it might be better the split them into a set of separate exports. This would mean imports could look like they are in the current master branch:

import { getFirstMatch, getSecondMatch, } from './utils.js';

rather than importing the whole of Utils.

Add ES6 style export to utils and change related utils imports
Update eslint rule for import extensions
@coveralls
Copy link

Pull Request Test Coverage Report for Build 453

  • 53 of 53 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 88.698%

Totals Coverage Status
Change from base Build 450: -0.01%
Covered Lines: 468
Relevant Lines: 483

💛 - Coveralls

getFirstMatch,
getSecondMatch,
} from './utils';
import Utils from './utils.js';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, but I don't understand, why we need to replace extracted getFirstMatch and getSecondMatch methods to the Utils. I thought all we needed is the ES2015 module export changes in utils.js and the importing part would have a .js added in the end. Do I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree this isn't ideal - it happens is because Utils is currently a class with static methods.
With 'pure' es6 you can't import methods separately from a class (I didn't even realise it was possible with webpack until I saw it here :) )

The ideal option - which I mentioned in the original post above - would be to update the utils.js file into separate exports rather than static methods on a class. I'll update the PR later so you can have a look and see if it's what you want (should be back to looking how it was before except internally utils will export each function rather than export one class)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, right. My mistake, sorry. I've forgotten about the class. Actually, having one export produces less code in the end, because after all each module has it's own wrapper when babel transpiles it to ES5. So, no worries then and we can use it like that.

@lancedikson lancedikson closed this Mar 2, 2019
@lancedikson lancedikson reopened this Mar 2, 2019
@lancedikson lancedikson merged commit a81b8c0 into bowser-js:master Mar 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.

None yet

3 participants