Breaking Change in latest versions

[jeremiahsmall]

Hi Todd, it looks like you intended to tag 0.2.0 prior to your recent changes, but might have committed it after instead?

Anyway, the change you committed a couple weeks ago are BC breaking for Carafe, so we currently need to lock to commit-ish 00818f4 for lack of a prior tag. Think you might want to tag that commit as something in the 0.1 range? While you're at it, you could consider adding tags to Github for the commits where you incremented the version in your package.json, such as 0.1.1 at commit-ish 58184a1.

[toddgeist]

Hi Jeremiah, Not sure what happened there. Possibly I didn't commit after the publish to npm. I have build and publish script that is supposed to do all of this. It clearly published to NPM, but possibly I forgot to commit the tag. I added a tag for that commit. Anyhow I think the better long term solution would be to resolve the incompatibilities. Or possibly merge the libraries, if that is possible. Thanks Todd

…

-- Todd Geist CEO, Geist Interactive 501 I South Reino Rd, #296 Newbury Park, CA 91320 todd@geistinteractive.com (805) 941-0435 [1479131529888_Outlines.png]

________________________________ From: Jeremiah Small <notifications@github.com> Sent: Friday, November 30, 2018 3:34 PM To: geistinteractive/fm-webviewer-bridge Cc: Subscribed Subject: [geistinteractive/fm-webviewer-bridge] Older Tags missing (#4) Hi Todd, it looks like you intended to tag 0.2.0 prior to your recent changes, but might have committed it after instead? Anyway, the change you committed a couple weeks ago are BC breaking for Carafe, so we currently need to lock to commit-ish 00818f4<00818f4> for lack of a prior tag. Think you might want to tag that commit as something in the 0.1 range? While you're at it, you could consider adding tags to Github for the commits where you incremented the version in your package.json, such as 0.1.1 at commit-ish 58184a1<58184a1>. [pasted_image_11_30_18__3_27_pm]<https://user-images.githubusercontent.com/814871/49320215-5230fc00-f4b5-11e8-840e-eac3af10d1d7.png> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#4>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AATVeIRFo7ICAkBzEseT5B_eajMH_zlYks5u0cChgaJpZM4Y8j65>.

[Luidog]

@toddgeist I would ask that you not merge the libraries. Carafe is an awesome project - @jeremiahsmall has done an awesome job - but I think it would be better for this project to remain a small project and Carafe to import this project as a dependency. I have a few projects planned that would leverage this project as a dependency. @jeremiahsmall please clarify if I am wrong, but Carafe is focused on managing dependencies / imports of web viewer integrations. It does not seem suitable to use in another npm project as a dependency.

[Luidog]

I think a resolution here might be to follow semantic versioning in this project or for Carafe to use stricter dependency management. From @jeremiahsmall's original comment it looks like there was a breaking change in 0.3.1 and his project updated automatically.

Side note - I want to take a second to both @toddgeist and @jeremiahsmall for contributing both their projects. they are both awesome and will serve to move our community forward.

[toddgeist]

the project is supposed to follow semantic versioning. The changes I recently made were not intended to be breaking. And did not break anything in my tests, but it is certainly possible.

The earlier versions are still on npm and you can lock to them for the time being.

Here is the one you can lock to 0.1.1
https://www.npmjs.com/package/fm-webviewer-bridge/v/0.1.1

You do not need a "tag" to do that. just depend on 0.1.1 in package.json

I would like to either resolve the breaking change or if it can't be resolved then republish as a 1.0.0

[toddgeist]

I changed the title of this issue since the missing tags are not really relevant.

[Luidog]

I did not see any breaking changes in my use of the later versions of this project. It would be helpful to know what exactly broke in @jeremiahsmall's project.

[Luidog]

I have begun writing tests and collecting code coverage on my fork of the project. My first attempt at writing tests leveraged mocha. After hitting a few blockers, I ended up switching to jest. I have found in my short time using it that jest might be the better fit for this project.

@toddgeist Would you consider adapting jest for this project's testing? Would you be willing to accept a PR once I get test coverage above a certain percentage?

[toddgeist]

sure

[jeremiahsmall]

Hi guys, sorry for not responding more promptly. @Luidog maybe before you start on test coverage, we should settle on the question of potentially consolidating the effort. Currently both this project and Carafe are very tiny/simple JavaScript packages. The main area of overlap is how they handle initial data.

If we could define a common API for that, then we could easily consider unifying. The main additional feature that Carafe.js has is the head loading ability, but this could be an opt-in thing.

In case you didn't already, take a look at Carafe.js https://github.com/soliantconsulting/carafe/blob/master/es6/lib/Carafe.js

This, and the bundle format, are the nub of Carafe. All the rest is just tooling.

[Luidog]

I am actually going to move forward with testing. I am already at 10 % and will propose a pull request somewhere around 80%. I think getting some testing in place would aid future development.

@jeremiahsmall Class based extensibility is a great idea. I really like it. Also the Head loading via headjs is an awesome feature. I could see Carafe working as a HOC using fm-webviewer-bridge.

[toddgeist]

There is no need for classes. Classes are a performance optimization when you are creating thousands of instances of an object. There is one object on the page.

Factory functions and HOCs are much better

[Luidog]

I knew that Todd would not like the Class approach 😆. We have had many conversations about this.

I agree there is no need for Classes currently. In the browser Classes compile down to factory functions anyway... but I still like to look at them 😜.

[Luidog]

I am happy to say that I was able to exceed the 80% goal without changing the project's source code base. @toddgeist please review Pull Request #6 . @jeremiahsmall it would be great to turn whatever broke for you into a test case.