Fix make dist and browserify issue require #572

2017-02-27T09:23:42Z

Liryna commented

2017-02-27 09:23:42 +00:00

(Migrated from github.com)

No description provided.

coveralls commented

2017-02-27 09:52:09 +00:00

(Migrated from github.com)

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

[![Coverage Status](https://coveralls.io/builds/10342404/badge)](https://coveralls.io/builds/10342404) Coverage remained the same at 94.624% when pulling **56f1f4e67cd36752fc2b6d3f168e5630a3fc570f on Liryna:master** into **5ae6b1965bfe3764656a96f536b356cd1586fec7 on SheetJS:master**.

SheetJSDev commented

2017-02-27 18:03:01 +00:00

(Migrated from github.com)

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?

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 commented

2017-02-27 20:16:53 +00:00

(Migrated from github.com)

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.😢

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 commented

2017-03-06 06:51:56 +00:00

(Migrated from github.com)

@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 commented

2017-03-10 15:48:33 +00:00

(Migrated from github.com)

Hi @SheetJSDev ,

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

Hi @SheetJSDev , Thank you for the changes ! Yes it worked for me on meteor 👍

SheetJSDev commented

2017-03-12 19:38:03 +00:00

(Migrated from github.com)

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 https://github.com/SheetJS/js-xlsx/issues/589

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 https://github.com/SheetJS/js-xlsx/issues/589