search

Lullabot / storybook-drupal-addon

Storybook addon to facilitate integrating Storybook with Drupal projects.
MIT License
39 stars 7 forks source link

Add _componentFileName to component request payload #41

Open agarzola opened 8 months ago

agarzola commented 8 months ago

Description

This PR updates the fetchStoryHtml.ts to add the _componentFileName request parameter in addition to (not a replacement of) _storyFileName in accordance with merge request 19 in cl_server, which deprecates _storyFileName in favor of _componentFileName.

Motivation

See Drupal issue 3387642.

e0ipso commented 8 months ago

I am hesitant about this change. This will make this addon incompatible with some versions of CL server. This would lead us to major version changes for both projects, which seems like a lot of hassle for changing the name of a private parameter.

Perhaps we can save this for when we are ready for a new major version. What do you think?

agarzola commented 8 months ago

@e0ipso That makes sense to me. Another alternative would be to pass both _storyFileName and _componentFileName, with a to-do to remove _storyFileName in the next major version. Would that make this PR more palatable in the short term?

e0ipso commented 8 months ago

I am still resistant of this change. This adds a certain level of risk (someone updates this plugin, but not the Drupal module), and I see no real benefit of renaming internal variable names and methods.

I agree that your proposed names make more sense, but this is not something that justifies the maintenance burden that supporting a matrix of compatibility versions carries.

Perhaps when we have plans to do a major update for other reasons, this should make it in?

Let me know if I am missing something. Have you started working on a different pattern library implementation?

agarzola commented 8 months ago

@e0ipso Thanks for reviewing. This PR sends both parameters (_componentFileName and _storyFileName) precisely to avoid the burden you mention. However, I just realized that I had failed to update the PR title and description, so I just updated both to more accurately represent this change set.

Can you take another look and let me know of any additional maintenance considerations that I may be missing?