-
Notifications
You must be signed in to change notification settings - Fork 16
Module support #7
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
base: master
Are you sure you want to change the base?
Conversation
|
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 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. |
|
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. |
|
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. |
|
Hello, I finished PR. I rebased on latest master. Then I addressed your thoughts:
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. |
It breaks multiple calls to `browserReport(callback)` Only callback from 1st call to `browserReport()` is executed.
|
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 ***@***.***> 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.
|
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 modulesbrowserReport&browserReportSync. Exampleor sipmpler
Hope you integrate this, if yes, please publish package on NPM.
Cheers