removing v1 embedding to fix compile on flutter 3.27 #1644
Conversation
abdelaziz-mahdy
changed the title
| @@ -1,3 +1,7 @@ | |||
| ## 8.1.6 | |||
There was a problem hiding this comment.
I believe you should Add an new version instead of removing this one, no?
There was a problem hiding this comment.
I don't know what you mean, I did add a new version?
If you are talking about v1 embedding it has been deprecated for.a while Now, it should be removed anyway
There was a problem hiding this comment.
I think I know what he mean, you didn't add specific new version on this changelog like the 8.1.5 one. So better you add it as 8.1.6 and the package owner should merge this pr then publish to pub.dev
There was a problem hiding this comment.
I'm not a maintainer but the changelog looks just fine to me.
There was a problem hiding this comment.
I've tested this on my setup, and everything works as expected. Great job! @abdelaziz-mahdy
There was a problem hiding this comment.
I don't know what you mean, I did add a new version?
Sorry, my bad... I did not have my coffee yet when I reviewed :)
There was a problem hiding this comment.
Bumping version is good until another PR with version bump is merged. It is up to the maintainer if he requires version bump as part of the PR. Personally I do the version bump in the release branch dedicated the version release. I know that for example Flutter community plugins require version bump (I've been through it and got hindered several times due to other PRs with version merging and having to resolve conflict and rebase).
Just merge this PR (🙏🏻 🙏🏻 🙏🏻) with or without version FFS.
|
The bug has a huge number of comments so I am adding some context here. My teammate on flutter removed v1 embeddings in this pr #1527 fixing the issue we filed #1526. It appears that in pull request #1453 the lines we removed were added back in. 5 days ago flutter released 3.27 which was the first stable release to not have the v1 embedding code so any user with this plugin and 3.27 would fail to compile. At first glance this pr appears to address the regression. |
fixing this #1643