Skip to content

Refactor code#38

Merged
mayashavin merged 8 commits into
masterfrom
refactor-code
Oct 6, 2019
Merged

Refactor code#38
mayashavin merged 8 commits into
masterfrom
refactor-code

Conversation

@mayashavin
Copy link
Copy Markdown
Contributor

  1. Remove unused code (uniq, union, extractOptions)
  2. Simplify code in utils
  3. Remove the use of State and Channel class, switch to use provide/inject.
  4. Fix unit test.
  5. Enable CldTransformation

@mayashavin mayashavin requested a review from arturkulig August 28, 2019 07:28
@arturkulig
Copy link
Copy Markdown
Contributor

I have read the code, checked-out onto the branch.
I run the tests - they work. ✔️

However the tests have one issue - you're using await Vue.nextTick(). That's guaranteeing that Vue performs two renders before check-ups. Removing it exposes empty image and test fails.

I also created a branch and a PR that added test for SSR - I have merged that onto your branch, added CldTransformation to that (cause I made the test for a repo state where it is removed) and that also failed.

This unfortunately brings the repo state to before one-pass. Your solution, as I have studied that, is really based on the same concept, but without jumping through hoops with observables guaranteeing that cld-image refreshes even if for some reason only child cld-transformation was triggered with new attributes.


I see you have removed merge function in favour of JS spreading. I didn't use it as Vue is meant to be used in set of a browsers where not all support the feature. In such case the feature is polyfilled with Object.assign. Covering the same set of browsers would mean including the polyfill. However the cloudinary core SDK already includes such function, so it'll be bundled in anyway. That approach is what I have previously established with Amir. Sure, we can change it - just letting you know why it was there and so you can


I didn't made utils into a single file to have a clear overview of what functions I have. I try to include single concept into a file. That might mean different things in JS unlike for example Java where that would mean a class - a function, a class, a module (set of functions that are connected with their purposes). Separating functions into files also prevents unused code to be included into a resulting bundle. I'd prefer you to explain why you changed it, so I can understand it and apply it in the future changes.

@mayashavin mayashavin merged commit 8ac038d into master Oct 6, 2019
@mayashavin mayashavin deleted the refactor-code branch October 6, 2019 20:19
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