magento / pwa-studio

🛠Development tools to build, optimize and deploy Progressive Web Applications for Magento 2.
https://developer.adobe.com/commerce/pwa-studio/
Open Software License 3.0
1.07k stars 683 forks source link

[feature]: Third parties should be able to tap in to talons #1954

Open zetlen opened 5 years ago

zetlen commented 5 years ago

Emerged from discussion about deprecating venia-drivers.

Description

We may not want to encourage devs to stub out or redirect our core dependencies, but we want them to be able to implement interceptors around Peregrine talons.

The clearest use case for this is analytics: a third party should be able to "tap in" to the useSearchResult hook, for instance, to trigger an analytics event about a search result. This is way better than making a developer thread analytics code all around their components, and it requires us to expose that interceptor capability to developers.

Not an ideal solution

The easiest way to do that would be to create a global event bus, and then permit injected libraries to subscribe to these global events. All Peregrine talons would trigger these events every time they run, which would allow subscribers to do whatever they want in response. That adds complexity and global state, and it doesn't allow third parties to modify or decorate the behavior of talons, but it would be simple. We should do a small research spike on it.

An ideal solution

The harder, but the most powerful and production-capable way to do it is by adding a special flag to the configureWebpack() function which enables modules to contribute interceptors to Peregrine talons. The API would be something like:

configureWebpack({
  special: {
    '@magento/venia-ui': {
      esModules: true,
      cssModules: true,
      rootComponents: true,
      upward: true
    },
    '@adobe/peregrine-ecom-analytics': {
      talons: true
    }
})

The hypothetical @adobe/peregrine-ecom-analytics package would have to adhere to the Peregrine filesystem convention, but in each file, it would need to expose a wrapper function into which the original talon implementation would be injected.

packages/@adobe/peregrine-ecom-analytics/lib/talons/useSignIn.js:

import { useTracker } from './useTracker';

export default function useSignIn(props, callParent) {
  const tracker = useTracker(process.env.ADOBE_ANALYTICS_ID);
  const model = callParent(props);
  return {
    ...model,
    async handleSubmit(...args) {
      const out = await model.handleSubmit(...args);
      tracker.trackEvent('signedIn', out);
      return out;
    }
  }
}

The plugin would be a façade over a NormalModuleReplacementPlugin which builds final modules from a sequence of wrappers supplied by the talons flagged dependencies. The above example would compose with other wrappers for all talons, in whatever order the special collection listed them.

This would enable a lot.

could all be pretty easy to layer over an app using this approach.

Shall we?

Please let us know what packages this feature is in regards to:

jimbo commented 5 years ago

This is very cool ideation. I'm intrigued by the NormalModuleReplacementPlugin approach, since it would offer a lot of customization while leaving the talons themselves pristine.

If we end up having to do the global event bus, it wouldn't be the end of the world though.

sirugh commented 5 years ago

Neat idea! If we go with an event bus we will likely end up with tons of emitter code sprinkled throughout, which detracts from readability and maintainability. With the interceptor pattern we only have to harden around the talon API which should be stable anyways.

fooman commented 5 years ago

I am guessing we would have some similar process to how the upward.yml files are currently read from all packages and then merged? For example webpack.extend.js in the package root folder and this is where we can add configureWebpack() ?

I am currently envisaging that in an ideal scenario yarn add @fooman/example is all that is needed to provide a PWA component that customises/builds on top of PWA Studio.

zetlen commented 4 years ago

@fooman That's a workflow we want to enable. We're starting with explicit declaration of the modules to incorporate. Implicit merging of modules is something we're used to in the Magento ecosystem, but it has some debuggability problems; we'd like to stick with our flags approach for now.

However, we would be interested in automation and helper tools which generate a lot of this explicit configuration code themselves!

fooman commented 4 years ago

Thanks @zetlen for getting back to me. I am not 100% sure that I understand the current plan. Are you saying for now we would have in my installation instructions yarn add @fooman/example followed by please find your webpack config file and add

    '@fooman/example': {
      talons: true
    }

in the special section? Agree that some helper script would be beneficial as hand editing files never goes well (missing comas, finding the right section, ...).

zetlen commented 4 years ago

That's the current plan. Hey, it sounds like you manage to avoid hand-editing text files in your usual workflow. Do you find you have to edit some XML? What are the biggest pitfalls in your own module installation process?

fooman commented 4 years ago

@zetlen I am currently only speaking from the standpoint of non PWA Magento extensions. And from there I can totally second the need for simplification. Currently to install an extension into Magento it's about 5 to 7 different steps (which can differ depending on what mode Magento itself is in - as an aside trying to fix that too bringing it down to 1 step).

Discovering that I can run composer config repositories.fooman composer https://example.com/ vs instructing please find your composer.json file, find the repository section and then add

{
    "type": "composer",
    "url": "https://example.com"
  }

provided a big simplification in the process, removed lots of friction and potential for errors (is it in the right location, is it still valid json, did it remove something that it shouldn't, arrays can be tricky to configure by hand). Your above comments reminded me of this.

So if we could include out of the box the ability to modify the webpack config via a configurable command I am all for it, ie yarn run talon-enable @fooman/example is better than the manual edit instructions.

zetlen commented 4 years ago

@fooman Thanks for taking the time to explain. One more question if you can:

How do you manage the priority order of installed modules? Do you edit config.php directly? Is there another tool or workflow you use? Or do you find that you just don't need to do it that often?

fooman commented 4 years ago

@zetlen editing config.php by hand is not recommended. Magento reads all etc/module.xml files in which you can define a sequence (ie this module should come after this, this and that one) and then writes out the sequence into the app/etc/config.php file (ie your hand editing would get overwritten). An example is here.

The module load order for Magento modules is very important as it influences things like when the database install script runs (ie you can only reference store_ids in your custom module after Magento core has added those tables), the fallback structure for themes and the underlying merging of xml files all depend on the load order (ie last one merged wins).

zetlen commented 4 years ago

Thank you, @fooman . From time to time I need my memory refreshed, and there's so much going on in our core teams that sometimes an external contributor has the clearest view.

sirugh commented 3 years ago

@magento export issue to JIRA project PWA as Story

github-jira-sync-bot commented 3 years ago

:white_check_mark: Jira issue https://jira.corp.magento.com/browse/PWA-1770 is successfully created for this GitHub issue.