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 browser build #417

Closed
wants to merge 2 commits into from
Closed

Conversation

curran
Copy link
Contributor

@curran curran commented Jan 28, 2024

Closes #69

Heavily inspired by the great work already done by @cfraz89 in #239, this PR is a slightly more surgical approach, introducing the bare minimum changes to get a browser build to work.

I have tested it locally using npm link and it does work!

One drawback of this implementation is that it still exposes the reference to the global Buffer. This makes the code changes to this project minimal, but shifts the burden to consumers to perform some nasty hackery to get it to work, namely this:

import * as prettierPluginSvelte from 'prettier-plugin-svelte/browser';
import { Buffer } from 'buffer';
globalThis.Buffer = Buffer;

I propose to make the change that removes the Buffer dependency separately, treating that as a separate issue that would be an enhancement to the browser build.

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this! Since import maps are standarized now, I think this is the right approach to externalize the modules and require people to use an import map or a bundler to fill the rest. Once the buffer PR is merged I'll look into this more closely.

package.json Show resolved Hide resolved
@@ -3,3 +3,5 @@ node_modules
.history
plugin.js
plugin.js.map
browser.js
browser.js.map
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: check how big the .js.map of the browser build is, and if it's big, then decide if it's worth it to include it (I don't think so)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, could leave it out.

This was referenced Jan 29, 2024
@@ -6,6 +6,8 @@
"files": [
"plugin.js",
"plugin.js.map",
"browser.js",
"browser.js.map",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
"browser.js.map",

@curran
Copy link
Contributor Author

curran commented Feb 9, 2024

Just want to give a heads up that I plan to publish this build as a separate package in NPM in the next week or so, so that I can use it in my app.

Please let me know if there are any blockers to merge that I can help with here. Thanks!

@dummdidumm
Copy link
Member

I wanted to do a few edits but wasn't able to, therefore opened #430

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.

usage in the browser
2 participants