Add _componentFileName to component request payload #41
Conversation
|
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? |
|
@e0ipso That makes sense to me. Another alternative would be to pass both |
| _componentFileName: string; | ||
| // Preserve backward compatibility with older versions of drupal/cl_server: | ||
| _storyFileName: string; | ||
| _drupalTheme: string; | ||
| _variant?: string; | ||
| _params: string; | ||
| } = { | ||
| _componentFileName: context.parameters.fileName, | ||
| _storyFileName: context.parameters.fileName, |
There was a problem hiding this comment.
I updated this PR to include both parameters, which preserves backward compatibility. See accompanying commit for cl_server, which deprecates (instead of dropping) _storyFileName.
This should make for a smoother transition.
|
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
changed the title
|
@e0ipso Thanks for reviewing. This PR sends both parameters ( Can you take another look and let me know of any additional maintenance considerations that I may be missing? |
Description
This PR updates the
fetchStoryHtml.tsto add the_componentFileNamerequest parameter in addition to (not a replacement of)_storyFileNamein accordance with merge request 19 incl_server, which deprecates_storyFileNamein favor of_componentFileName.Motivation
See Drupal issue 3387642.