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

removed unused options and update Readme #56

Merged
merged 4 commits into from Dec 2, 2019

Conversation

@mayashavin
Copy link
Contributor

mayashavin commented Nov 18, 2019

  1. As discussed with @asisayag2 , remove unnecessary install options (by string, by boolean), leaving only three left: Array of components or Object of components (with options to rename) or everything.
  2. Refactor index.js and plugin.js
  3. Update README.md
@mayashavin mayashavin requested review from arturkulig and asisayag2 Nov 18, 2019
CldContext,
CldImage,
CldVideo
const allComponents = { CldContext, CldImage, CldVideo, CldTransformation, CldPoster };

This comment has been minimized.

Copy link
@arturkulig

arturkulig Nov 19, 2019

Collaborator

allComponents is not used outside of install function call, so it'd be more readable to place it there.

I wouldn't, however, even point that out, but there is also potential issue with pre-tree-shaking static code analysis. I'd suspect that named imports could fail to import just a single export (which should be easy as this file could really only just reexport other file exports) as this variable uses all of them and is not what it is meant to be without all components. Might be that minifiers could also reject that variable if install function declared here was not used and rejected also, but I'm not sure of that.

Reason for wanting to be simple in the first place was that we are not in control of any tooling that is and will be, so cannot guarantee that those tools will be capable of such smart conclusions like when to strip cloudinary-vue bundle of unnecessary components even if in this entry file, in this line they are all used.

Keeping this file containing just reexports had this advantage that it was simple to conclude which files should be attached to an app bundle for a static code analysis tool. That is why plugin file was importing those files. As I understand you want to make users to import each component and use that to specify installation, so plugin may not import cld* components in this setup.

This comment has been minimized.

Copy link
@mayashavin

mayashavin Nov 19, 2019

Author Contributor

The best way to tree shaking is to import dynamically the component upon request (all Components), but I haven't figured out how to do it yet without configure webpack further so for now it should be ok :). It's possible actually :). And yes I prefer user to be the one who import each component per usage.

About the allComponents, as I don't want to re-import the same components inside plugin.js, I prefer to have it there and keep the install function simpler with one logic rather than multiple if...else :). The better way is to remove this variable and add directly when plugin.install is called with no components (line 12). Wdyt?


const Cloudinary = {
install: (Vue, options = {}) => {
plugin.install.call(plugin, Vue, options.components ? options : { ...options, components: allComponents })

This comment has been minimized.

Copy link
@arturkulig

arturkulig Nov 19, 2019

Collaborator

.call is redundant

README.md Outdated

Install the package in your project with

```bash
npm install cloudinary-vue
//OR

This comment has been minimized.

Copy link
@arturkulig

arturkulig Nov 19, 2019

Collaborator

just a tip really, nothing serious: single-line shell comments start with #

This comment has been minimized.

Copy link
@mayashavin

mayashavin Nov 19, 2019

Author Contributor

Thanks :)

CldContext,
CldImage,
CldVideo
Cloudinary as default,

This comment has been minimized.

Copy link
@arturkulig

arturkulig Nov 19, 2019

Collaborator

I'd suggest changing this to

Cloudinary as default,
Cloudinary,

Just to avoid a bug reports when someone contrary to docs decides to import as

import { Cloudinary, CldImage } from "cloudinary-vue";
Copy link

jackieros left a comment

@mayashavin , @asisayag2

There's no link from the Vue Readme to the Vue documentation.
(Just a general link to the entire doc website at the bottom of the page under "Additional Resources")

The readme should clarify that it provides basic installation and usage information, but that users should see the complete Vue SDK documentation: https://cloudinary.com/documentation/vue_integration

I would say the best place for a note & that link would be in the top section of the readme before the Installation section, but the entire top section should really be rewritten. For example, it only talks about images and it doesn't say anything specific about Vue. I think that whole paragraph should be rewritten to be one-line about Cloudinary & then a few sentences about the Vue SDK. You can take what we have in the first paragraph of our Vue guide, or suggestion something else?

Additionally, There's also a "Documentation" section at the very bottom of the page with info about styleguidist - Do we want that in the readme?

@mayashavin mayashavin merged commit 6a62756 into master Dec 2, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@mayashavin mayashavin deleted the remove-unused-install-options branch Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.