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

Fix make dist and browserify issue require #572

Closed
wants to merge 4 commits into from

Conversation

Liryna
Copy link

@Liryna Liryna commented Feb 27, 2017

No description provided.

@coveralls
Copy link

coveralls commented Feb 27, 2017

Coverage Status

Coverage remained the same at 94.624% when pulling 56f1f4e on Liryna:master into 5ae6b19 on SheetJS:master.

@SheetJSDev
Copy link
Contributor

Hello @Liryna !

The breaks in the require statements are used to trick browserify into not including those files. If require is not available (like in the browser) none of those dependencies are pulled in. When you are using browserify do you find it pulls in an FS shim or other shims?

@Liryna
Copy link
Author

Liryna commented Feb 27, 2017

Hi @SheetJSDev ,

It is not really browserify in my case, it is meteor that do his magic trick to detect which JS are used by scanning the require to send to the client side. With the break, it seems like meteor is unable to see it and the jszip is for example not sent and therefore missing at runtime.😢

@SheetJSDev
Copy link
Contributor

@Liryna the relevant changes were applied to un-break the require statements. Please check if it works. Keep the PR open though, since you also made some changes to the makefile I also want to revisit the build infrastructure

@Liryna
Copy link
Author

Liryna commented Mar 10, 2017

Hi @SheetJSDev ,

Thank you for the changes ! Yes it worked for me on meteor 👍

@SheetJSDev
Copy link
Contributor

We also folded the ODS logic into xlsx.js and reworked the cpexcel build to omit the unnecessary require, so I think we settled the require issues.

GNU sed doesn't accept a space between the -i and the specified extension. The BSD sed appears to accept it, even though the manpage states the expected form is -i extension, so we will fix that as part of #589

@SheetJSDev SheetJSDev closed this Mar 12, 2017
SheetJSDev added a commit that referenced this pull request Mar 13, 2017
- decode sheet name for XLSX and XLML (fixes #203 h/t @rocketmonkeys)
- XFExt (fixes #298 h/t @aetna-softwares @aimcom @baharudinafif)
- handle truly empty `<is>` elements (fixes #506 h/t @asksahil)
- pin version numbers for dependencies (fixes #469 h/t @nhtera)
- sed usage fix (see #572 h/t @Liryna)
- fix hex2RGB substr indices (fixes #294 h/t @kamorahul)
- removed stale typescript files (see #442)
- reworked shift formula regex (fixed #551 h/t @SheetJSDev)
- README note on webpack codepage suppression (fixes #438 h/t @rusty1s)
- README note on WTF (fixes #487 h/t @livesoftware)
saarCiklum pushed a commit to Folcon/js-xlsx that referenced this pull request Aug 18, 2020
- decode sheet name for XLSX and XLML (fixes SheetJS#203 h/t @rocketmonkeys)
- XFExt (fixes SheetJS#298 h/t @aetna-softwares @aimcom @baharudinafif)
- handle truly empty `<is>` elements (fixes SheetJS#506 h/t @asksahil)
- pin version numbers for dependencies (fixes SheetJS#469 h/t @nhtera)
- sed usage fix (see SheetJS#572 h/t @Liryna)
- fix hex2RGB substr indices (fixes SheetJS#294 h/t @kamorahul)
- removed stale typescript files (see SheetJS#442)
- reworked shift formula regex (fixed SheetJS#551 h/t @SheetJSDev)
- README note on webpack codepage suppression (fixes SheetJS#438 h/t @rusty1s)
- README note on WTF (fixes SheetJS#487 h/t @livesoftware)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants