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

Crx #119

Merged
merged 1 commit into from
Jun 1, 2019
Merged

Crx #119

merged 1 commit into from
Jun 1, 2019

Conversation

ZJONSSON
Copy link
Owner

Closes #118
Add support for crx files in methods Parse() ParseOne() and Open.buffer()

@silverwind
Copy link

silverwind commented May 20, 2019

Some suggestions:

  1. Make crxHeader available on the result of Open, e.g. directory.crxHeader.
  2. Any specific reason to not support CRX files on Open methods other than buffer? While I'm happy with it in my current use case, I think the buffer slicing could be extracted to its own function and used on all Open methods.

@ZJONSSON
Copy link
Owner Author

  1. good point - will do
  2. it will require an extra roundtrip to the server to inspect the header of the file. A regular zip file would not require inspecting the header. We could have an optional findStartOffset: true that would trigger header inspection that searches for start of the zip file and add the startOffset to any offset references in the dictionary

@silverwind
Copy link

Ah, I see. For methods that fetch a remote file, it's probably not worth the overhead. For Open.file it may be, but I guess we could also just leave it unsupported there given that reading a file (or parts of it) into a buffer is trivial.

@ZJONSSON
Copy link
Owner Author

I'm thinking we should add the crx option to all method before merging. I have a first cut, will try to finish soon

@silverwind
Copy link

👍 probably best to do it on all methods via opt-in.

@ZJONSSON ZJONSSON force-pushed the crx branch 3 times, most recently from 53cb540 to 83ec0c8 Compare May 31, 2019 13:10
@ZJONSSON
Copy link
Owner Author

@silverwind this is now complete, can you please review

@silverwind
Copy link

Sorry for being so late here. Looks fine to me, thanks again!

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.

Support CRX Package Format
2 participants