browser-report icon indicating copy to clipboard operation
browser-report copied to clipboard

Module support

Open srigi opened this issue 8 years ago • 5 comments

Hi, I added module support for AMD and module bundlers. I tested my changes with RequireJS (sample page included) and Webpack (working, however the code completion is not working).

I implemented the UMD like style inspired by platform.js. I made some changes for module bundlers specifically - I wanted that default exported module is browserReport() function. But library also exports named modules browserReport & browserReportSync. Example

import { browserReport, browserReportSync } from 'browser-report';

or sipmpler

import browserReport from 'browser-report';

Hope you integrate this, if yes, please publish package on NPM. Cheers

srigi avatar Jan 18 '17 15:01 srigi

Thank you for this work. I've wanted module support for a while now, but I have not had the time to work on it. I'd love to merge this pull request, however, there are some issues I'd like resolved.

Since you renamed the entry script, you broke the npm test script. Please update the test script in package.json, run npm test, and fix the issues reported.

I also won't accept pull request that mixes code changes and formatting changes in one request. I understand why you want to change the sample page from tabs to spaces, and I'm even ok with it, but that change needs to be in a separate commit.

Also, I'd like to export the browserReport() function with a sync() property like the node-glob library.

In your comment you mentioned UDM, so I expected the code to look like the last part of [returnExports.js][returnExpoerts.js], but it does not.

Lastly, I'd follow the lory.js pattern for module support. I especially like how they document installing with node, consuming it as an ES2015 module, consuming it as a commonJS module, and installing with bower. The CDN option is nice too.

I know this adds up to a lot of changes, but you inspired me to look into it and this is the direction I'd like to go.

keithws avatar Jan 19 '17 08:01 keithws

Note, I know my whitespace was all over the place in this project, so I standardized on four spaces in a recent commit. Please rebase.

keithws avatar Jan 19 '17 08:01 keithws

Hey, thanks for looking into it. I will update source soon. I wanted to preserve current coding style, but I think that my editor changed that upon saving file. I will be careful about that.

srigi avatar Jan 19 '17 15:01 srigi

Hello,

I finished PR. I rebased on latest master. Then I addressed your thoughts:

  • all tests passing
  • code-style preserved
  • browserReport.sync added
  • follows returnExports.js from UMD

The last point - build & docs like lory.js is not addressed in this PR. I'm happy to refactor this library into modules which are then builded by Webpack2. But this should be done in another PR.

Please let me know what you think. I will appreciate if you publish this to NPM.

srigi avatar Feb 08 '17 13:02 srigi

Thank you! I appreciate your hard work. I don't have time to review this now, but I'll make time this week.

On Feb 8, 2017, at 5:51 AM, Srigi [email protected] wrote:

Hello,

I finished PR. I rebased on latest master. Then I addressed your thoughts:

all tests passing code-style preserved browserReport.sync added follows returnExports.js from UMD The last point - build & docs like lory.js is not addressed in this PR. I'm happy to refactor this library into modules which are then builded by Webpack2. But not this should by done in another PR.

Please let me know what you think. I will appreciate if you publish this to NPM.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

keithws avatar Feb 08 '17 17:02 keithws