Contrib.social

Clarify identifiers in ServerEndpointController

Open on Drupal.org →
Created on 15 September 2023, over 1 year ago
Updated 26 September 2023, over 1 year ago

Problem/Motivation

ServerEndpointController.php makes several references to “story/stories” and “Storybook” in variable and method names, as well as some comments and error messages. When coming across this code, it is easy to interpret it as meaning that cl_server is in fact designed to be used only with Storybook.js, but this is not actually the case. This module is intended to be agnostic with regard to which component library is used to preview components, and its code should reflect this.

Additionally, judging by the name of the findExtensionName method, it is easy to get the impression that it identifies the filename extension of the file whose path is provided to it. This method, however, is used to identify the Drupal extension (i.e. module or theme) from which a component is being requested.

Proposed resolution

Make the following changes in src/Controller/ServerEndpointController.php:

  1. Rename $story_filename to $component_filename
  2. Update message passed to ComponentNotFoundException constructor.
  3. Rename _storyFileName parameter to _componentFileName.
  4. Rename findStoryFile to findComponentFile
  5. Update docblocks and code comments to replace/remove references to Storybook and stories.
  6. Rename findExtensionName to findDrupalExtensionName
  7. Add a docblock to findDrupalExtensionName.

Remaining tasks

User interface changes

None.

API changes

  1. The following public methods will be renamed:
    • findStoryFile -> findComponentFile
    • findExtensionName -> findDrupalExtensionName
  2. The _storyFileName query string parameter will be renamed to _componentFileName.

Data model changes

None.

This will require a change in storybook-drupal-addon as well. See PR #41.

✨ Feature request
Status

Needs review

Version

2.0

Component

Code

Created by

🇺🇸United States agarzola

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @agarzola
  • @agarzola opened merge request.
  • Status changed to Needs review over 1 year ago
  • Comment over 1 year ago →
    🇺🇸United States agarzola

    Merge request is ready for review, as is PR #41 of storybook-drupal-addon. These should be released together and it may be prudent to bump the Storybook addon’s major version, since the PR introduces a breaking change.

  • Comment over 1 year ago →
    🇺🇸United States agarzola

    Would it be better to deprecate _storyFileName instead of outright replacing it?

  • Comment over 1 year ago →
    🇺🇸United States agarzola

    I pushed a commit that deprecates _storyFileName instead of outright removing it. I also updated the storybook-drupal-addon PR to include both parameters. This issue is again ready for review.

  • Status changed to Postponed over 1 year ago
  • Comment over 1 year ago →
    e0ipso Can Picafort

    I think everything here makes sense, but I don't see it enough reason to create a new major version. See https://github.com/Lullabot/storybook-drupal-addon/pull/41 for more context.

    I am postponing for now.

  • Status changed to Needs review over 1 year ago
  • Comment over 1 year ago →
    🇺🇸United States agarzola

    That is a valid concern and one that I agree with, which is why I updated both the MR for cl_server and the PR for storybook-drupal-addon to deprecate (as opposed to replacing) _storyFileName in favor of _componentFileName. I think the mix up is due to the fact that I had not updated the PR title and description in GitHub, both of which still referenced replacing _storyFileName. I have updated the title and description to more accurately represent the changes proposed here.

    Let me know if deprecating the old parameter is not enough to ease the burden of maintenance you referenced in the PR.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024