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

Jason/remove bom #70

Closed
wants to merge 3 commits into from
Closed

Jason/remove bom #70

wants to merge 3 commits into from

Conversation

jhnstn
Copy link

@jhnstn jhnstn commented Jun 30, 2014

Browserify will remove Byte Order Marks when it bundles a package.
see browserify/browserify#313 for some discussion on this.

But shiming the same file preserves the BOM and moves it down from the first character in the file.
This makes it difficult to remove with build tools.

This PR removes BOMs from packages before they are shimmed. I didn't add any configuration flag since browserify itself doesn't seem to either.

@thlorenz
Copy link
Owner

thlorenz commented Jul 1, 2014

Could you please clean that up? Leave my white-space as is please. Only apply the changes that are absolutely necessary to implement and test this feature.

Thanks.

@jhnstn
Copy link
Author

jhnstn commented Jul 1, 2014

I reverted the white space changes.
The test is https://github.com/jhnstn/browserify-shim/blob/jason/remove-bom/test/shim/shim-bom.js
and its fixture https://github.com/jhnstn/browserify-shim/blob/fa189acb631f6716d5fb1e1f1ab8695beded75ec/test/shim/fixtures/bom.js

Unfortunately it's difficult to see the BOM in the shim. I can add a test to make sure the fixture doesn't lose the one it has.

@thlorenz
Copy link
Owner

thlorenz commented Jul 1, 2014

Ok, now I can actually see what you are doing, thanks.

I'm not sure that this belongs with browserify-shim though. Correct me if I'm wrong, but wouldn't this have to be added to EVERY transform that wraps the content?

Maybe it'd be better to make browserify remove it even it it's not at the beginning of the file (due to transfomations) or even better have browserify fix the files before passing them on to the transforms.

cc @substack

@jhnstn
Copy link
Author

jhnstn commented Jul 1, 2014

I agree that this should not be in browserify-shim. closing

@jhnstn jhnstn closed this Jul 1, 2014
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.

None yet

2 participants