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

Modules 2.0 #3152

Merged
merged 73 commits into from Jul 15, 2019
Merged

Modules 2.0 #3152

merged 73 commits into from Jul 15, 2019

Conversation

filrak
Copy link
Collaborator

@filrak filrak commented Jun 27, 2019

Related issues

closes #3144

Short description and why it's useful

New modules API. Motivations can be found on #3144

Screenshots of visual changes before/after (if there are any)

Which environment this relates to

Check your case. In case of any doubts please read about Release Cycle

  • Test version (https://test.storefrontcloud.io) - this is a new feature or improvement for Vue Storefront. I've created branch from develop branch and want to merge it back to develop
  • RC version (https://next.storefrontcloud.io) - this is a stabilisation fix for Release Candidate of Vue Storefront. I've created branch from release branch and want to merge it back to release
  • Stable version (https://demo.storefrontcloud.io) - this is an important fix for current stable version. I've created branch from hotfix or master branch and want to merge it back to hotfix

Scope

  • New modules API
  • Create independent lerna package for @vue-storefront/module
  • Module template
  • Module npm package boilerplate
  • Hooks API
  • Define Hooks
  • Unit tests
  • Rewrite core modules to new API
  • Add depreciation note on old modules via Logger
  • Build @vue-storefront/module as npm package in vanilla js and publish

@patzick patzick added the not ready for merge PR is holded. Needs some clarifications or things that need to be finished. label Jun 28, 2019
@pkarw
Copy link
Collaborator

pkarw commented Jun 29, 2019

I like this idea a lot! Really! Let's make it happen in 1.11.

Some insights from my side:

  • @patzick suggested to have the events definitions on the module level - I like this idea,
  • we don't have any mechanism to unbind the events - isn't there a risk that one function will be added and then executed multiple time if the user won't be avare of this consequence? I mean when you add the listener in the mounted() - it will be added each time the component is mounted. I think it would be safe to add kind of hashing / safety key for that (maybe an optional key like listenerUniqueIdentifier so the user could control - for the selected event listeners if they're added always once or multiple times) ? @filrak

@filrak
Copy link
Collaborator Author

filrak commented Jul 1, 2019

@patzick suggested to have the events definitions on the module level - I like this idea,

You mean to let people add their own hooks inside module? I think making factory methods for hooks would public be sufficient in that case. @patzick WDYT?

@pkarw regarding your concerns about hooks. Imho we shouldn't have any under the hood checks like this:

  • first: hooks are generally not meant to be used in components (but ofc they can be)
  • second: depending on situation user might want to run hook from a given component multiple times (e.g. while placing an order). If he/she wants to make it a one-timer it's straightforward to add some flag inside hook function (onHook(() => { if(flag) something() })) to have this feature. Imho iImho we shouldn't expand our API for things that are easily doable for the user - It's another thing to maintain and think about in terms of breaking changes yet gives us nothing in return.

@pkarw
Copy link
Collaborator

pkarw commented Jul 1, 2019

@filrak OK, thanks for the comment

Copy link
Collaborator

@pkarw pkarw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to say: great job :-)

const storeView = currentStoreView()
const dbNamePrefix = storeView.storeCode ? storeView.storeCode + '-' : ''

initCacheStorage('categories')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should refactor this to ‚StorageManager.set’ - to have the same syntax like above (this function does exactly the same just for different storeName). The attributes store is the only one still needed (we can remove the other ones)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to remove categories and products keep only attribbutes

Copy link
Collaborator

@pkarw pkarw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really love these new modules. Nothing to add ❤️❤️

Copy link
Collaborator

@patzick patzick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice feature! :)

Just mostly removing unused code or correcting tests before we merge that in. Left few comments (and questions) for cleanup.

core/app.ts Outdated Show resolved Hide resolved
core/app.ts Outdated Show resolved Hide resolved
core/lib/module/index.ts Outdated Show resolved Hide resolved
core/mixins/composite.js Outdated Show resolved Hide resolved
core/modules-entry.ts Outdated Show resolved Hide resolved
packages/module/.gitignore Outdated Show resolved Hide resolved
packages/module/tests.spec.ts Outdated Show resolved Hide resolved
filrak and others added 6 commits July 11, 2019 12:28
Co-Authored-By: Patryk Tomczyk <13100280+patzick@users.noreply.github.com>
Co-Authored-By: Patryk Tomczyk <13100280+patzick@users.noreply.github.com>
Co-Authored-By: Patryk Tomczyk <13100280+patzick@users.noreply.github.com>
Co-Authored-By: Patryk Tomczyk <13100280+patzick@users.noreply.github.com>
Co-Authored-By: Patryk Tomczyk <13100280+patzick@users.noreply.github.com>
@pkarw pkarw added this to the 1.11.0-rc.1 milestone Jul 11, 2019
@filrak filrak changed the title [ WIP ] Modules 2.0 Modules 2.0 Jul 11, 2019
Copy link
Collaborator

@pkarw pkarw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is cool. Let's merge this in:

  • only unit tests needs to be improved (one is failing)
  • remove the initCacheStorage from catalog/index.ts for categories and products please - it's no longer required

@patzick
Copy link
Collaborator

patzick commented Jul 15, 2019

@filrak please fix tests so we could merge this PR :)

@filrak filrak merged commit cd621b8 into vuestorefront:develop Jul 15, 2019
@filrak
Copy link
Collaborator Author

filrak commented Jul 15, 2019

@patzick fixed by @lukeromanowicz ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not ready for merge PR is holded. Needs some clarifications or things that need to be finished.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants