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

Revert "use J as a binary instead of loading the module ( 120 + MBs )" #89

Merged
merged 1 commit into from
Jun 27, 2016

Conversation

tommygnr
Copy link
Contributor

@tommygnr tommygnr commented Jun 14, 2016

This reverts commit 6e35e0a made in #22

As I mentioned in a comment on that pull request the way npm v3 flattens dependencies means we can no longer rely on the j binary being in a consistent location relative to this package. I also think executing J in a separate process is the wrong way to go.

This change necessarily breaks #37 as well however without it textract can't be used as a dependency within another project to extract text from spreadsheets

@dbashford
Copy link
Owner

I'd rather at least check to see if it can be found either at the project root or where it's looking now before falling back to loading it in.

And if executing things in another process was wrong, textract has bigger issues :)

@dbashford
Copy link
Owner

FYI, just started a new job this week so may be until the weekend before I get back to it.

@tommygnr
Copy link
Contributor Author

And if executing things in another process was wrong, textract has bigger issues :)

Heh. I was only referring to running j in another process as it is a javascript library. Other binaries are a different story :-)

Other things I would say to support this change are:

  • memory consumption of j has been drastically reduced since this change was made 2 years ago
  • running j in a separate process is much slower and error prone

@dbashford
Copy link
Owner

Played with this a little, happy to go back.

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.

2 participants