Skip to content

Conversation

@srigi
Copy link

@srigi srigi commented Jan 18, 2017

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

@keithws
Copy link
Owner

keithws commented Jan 19, 2017

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
Copy link
Owner

keithws commented Jan 19, 2017

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.

@srigi
Copy link
Author

srigi commented Jan 19, 2017

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
Copy link
Author

srigi commented Feb 8, 2017

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.

Igor Hlina added 2 commits February 8, 2017 15:19
It breaks multiple calls to `browserReport(callback)`
Only callback from 1st call to `browserReport()` is executed.
@keithws
Copy link
Owner

keithws commented Feb 8, 2017 via email

@keithws keithws self-assigned this May 11, 2017
@keithws keithws added this to the 3.0 milestone May 11, 2017
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.

2 participants