search

exponea / exponea-ios-sdk

MIT License
19 stars 27 forks source link

Issues with the new HTML parsing feature #41

Closed username0x0a closed 1 year ago

username0x0a commented 1 year ago

I've identified another couple of issues coming with the introduced HTML parsing in the SDK.

For one, I've just noticed that the repository now contains a SwiftSoup.xcframework in a binary form, almost 40MBs of binaries! That's something I'd really prevent from happening in the first place, not just because of some repository rule to not contain (binary) files over 1MB in general, but also as it forces anyone linking the Swift package to check out this file, mainly on every CI build for example – that's all unnecessary traffic as we have to check out the SwiftSoup package repository as well due to it being a dependency. I can't find a good reason this is included & referenced by the included Xcode project as it should be probably kept there as a Swift package dependency as well.

The worse part is, even removing it now would probably not help very much as it will be kept in the GIT history, so as long as SwiftPM does a full repository checkout of all packages, it will probably stick this way forever. Being it my repository, I'd probably revise all the recent commits with it, drop it & create an alternative branch flow with it omitted, but that really needs someone handling GIT properly for that to result in a non-harming state (but it's possible, tho, even without no discomfort for 3rd-parties 👍 I could give you some hints).

Secondly, I haven't really checked the Exponea code recently, but what I found in the related HtmlNormalizer.swift is really a poorly handled code – throwing exceptions I'm not really sure we can always do something about, thus not being sure about its safety. What concerns me the most is a use of force unwrapping which is (in our own project) prohibited, forcing us to work with more sane flows (mostly early guarding in these cases). Additionally, it gives me the impression this dependency is only there to do some minor tweaks (fixing issues in the backend-generated code to display?), not really feeling like the cost/benefit of using it being very positive.

I'd welcome to see an improvement in these things around. 🙏

adam1929 commented 1 year ago

Thank you @username0x0a for your reaction. SwiftSoup has been added as xcframework because of Carthage package manager, unfortunately yes it takes more space that would be nice. We will work on tips you mentioned. Thank you again