-
-
Notifications
You must be signed in to change notification settings - Fork 32
feat(build): initial support for browser standalone build #166
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: ft/browser_build
Are you sure you want to change the base?
Conversation
|
Love this!! |
K-Kumar-01
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR @29217321 🚀
I am approving the PR. Just added few comments. Can you please clarify them?
f9949d0 to
5067192
Compare
Signed-off-by: 29217321 <[email protected]>
Signed-off-by: 29217321 <[email protected]>
Code reviewFound 4 issues: 1. Buffer polyfill missing from README documentationThe README's browser usage example (lines 213-216) shows polyfills for Issue: Users following the README will encounter runtime errors like Evidence:
Lines 213 to 216 in 090b23b
Fix: Add the Buffer polyfill to the README example: if (typeof Buffer === 'undefined') {
window.Buffer = {
from: function(arr) { return new Uint8Array(arr); },
isBuffer: function() { return false; }
};
}2. Outdated documentation contradicts new browser support implementationThe README contains a note stating "Browser support is planned for future releases" but this PR implements browser support with comprehensive documentation (lines 174-291). Issue: This creates confusion for users who read extensive browser build documentation earlier in the README, then see a note saying it's not yet available. The note should be removed or updated to reflect that browser support is now implemented. Line 609 in 090b23b
3. Missing cleaner plugin in browserConfigThe Issue: When building browser-only ( Lines 56 to 84 in 090b23b
Fix: Add the cleaner plugin to browserConfig: cleaner({
targets: ['./dist/']
}),4. Manual polyfills still required despite
|
| <!-- Polyfills for Node.js globals (required) --> | |
| <script> | |
| if (typeof global === 'undefined') window.global = window; | |
| if (typeof process === 'undefined') window.process = { env: {} }; | |
| </script> |
🤖 Generated with Claude Code
- If this code review was useful, please react with 👍. Otherwise, react with 👎.
|
@29217321 we appreciate your patience, our apologies that the claude code review comments were not posted. Can you address the above, and for each of these add a comment with the commit hash or explain why you think the comment should not be addressed? I want to get this out ASAP and have someone/claude work on updating the React example. cc @K-Kumar-01 |
nicolasiscoding
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address claude code comments and add commit hashes for each item fixed, or an explanation why it may not be needed
Signed-off-by: 29217321 <[email protected]>
|
@nicolasiscoding Thanks for the comments, fixed 1-3 and added a comments to explain why is polyfills still required now, please check out. |
This PR introduces a standalone browser build that bundles all dependencies into a single file, enabling direct usage of html-to-docx in browser environments without any build tools or module bundlers.
Usage Example:
Testing
Run> npm run build
Run> npm run test:browser
Open [http://localhost:8080/tests/test_browser.html]