Skip to content

[StimulusBundle] Skip mapping .ts controller if .js version is available #2702

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from
Open

[StimulusBundle] Skip mapping .ts controller if .js version is available #2702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/StimulusBundle/src/AssetMapper/ControllersMapGenerator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,22 @@ private function loadCustomControllers(): array

$controllersMap = [];
foreach ($finder as $file) {
// Skip .ts controller if .js version is available
Copy link
Member

Choose a reason for hiding this comment

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

Seen the code just belove, what about we sort files by name ascending (.js would be before .ts)

That way:

foo-bar-controller.js
foo-bar-controller.ts
foo_bar-controller.js
foo_bar-controller.ts

would only register the first one in the controller map, sa they would all have "foo-bar" as name.

We could

  • fix your problem
  • avoid open files when not needed
  • enforce same order / file content
$finder->in($this->controllerPaths)
            ->files()
+            ->sortByName()
            ->name(self::FILENAME_REGEX);

        $controllersMap = [];
        foreach ($finder as $file) {

             $name = $file->getRelativePathname();
             // ... name / preg match / replace / ..
             
+            if (isset($controllersMap[$name]) {
+                continue
+             }
             
             $asset = $this->assetMapper->getAssetFromSourcePath($file->getRealPath());
             // ...

Copy link
Author

Choose a reason for hiding this comment

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

I had a similar idea but later found out that controllers should be overridable. There is a test which checks for this feature:

ux/src/StimulusBundle/tests/AssetMapper/ControllersMapGeneratorTest.php

Line 115 in 61819ae

$this->assertStringContainsString('hello-controller.js override', file_get_contents($helloController->asset->sourcePath));

So skipping previously mapped controllers with the same name (if (isset($controllersMap[$name])) continue;) breaks that test since it doesn't allow controllers to be overridden.

And since controllers can be overridden, we probably don't want to sortByName() as that may break the override order given in the controller paths.

For example, given the following config/packages/stimulus.yaml:

# config/packages/stimulus.yaml
stimulus:
    controller_paths:
        - '%kernel.project_dir%/controllers_z'
        - '%kernel.project_dir%/controllers_a'

Then controllers_a/hello-controller.js would override controllers_z/hello-controller.js. But if we call sortByName() then controllers_z/hello-controller.js would override controllers_a/hello-controller.js. So devs could no longer specify controller override order in stimulus.yaml.

When you say:

  • avoid open files when not needed

Are you referring to the file_exists call? I haven't looked at the PHP source code but according to this page: The file_exists doesn’t actually open the file. It just checks the file system to see if the file or directory is there where you specified. I'm assuming file_exists queries the filesystem instead of trying to allocate a file descriptor/read the file, so it shouldn't lock the file or otherwise consume resources. Also the loadCustomControllers function is called once, either when running console asset-map:compile or when building the dev/test cache, so any performance impact should be negligible.

Another nice thing about if ($file->getExtension() === 'ts' && file_exists()) solution is the behaviour is explicit and localized to those few lines, so the loadCustomControllers function is easier to understand and refactor.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer the solution if ($file->getExtension() === 'ts' && file_exists()) which is more explicit than a sortByName() that works "by miracle" (I mean, without showing the intention behind)

Copy link
Author

Choose a reason for hiding this comment

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

When you say:

avoid open files when not needed

Ah, I think I know what you mean now, i.e. skipping previously mapped controllers would bypass

ux/src/StimulusBundle/src/AssetMapper/ControllersMapGenerator.php

Line 80 in 61819ae

$content = file_get_contents($asset->sourcePath);

The file_exists() solution will also avoid opening files unecessarily.

if ('ts' === $file->getExtension() && file_exists(substr($file->getRealPath(), 0, -2).'js')) {
continue;
}

$name = $file->getRelativePathname();
// use regex to extract 'controller'-postfix including extension
preg_match(self::FILENAME_REGEX, $name, $matches);
$name = str_replace(['_'.$matches[1], '-'.$matches[1]], '', $name);
$name = str_replace(['_', '/', '\\'], ['-', '--', '--'], $name);

$asset = $this->assetMapper->getAssetFromSourcePath($file->getRealPath());
if (!$asset) {
throw new \RuntimeException(\sprintf('Could not find an asset mapper path that points to the "%s" controller.', $name));
}

$content = file_get_contents($asset->sourcePath);
$isLazy = preg_match('/\/\*\s*stimulusFetch:\s*\'lazy\'\s*\*\//i', $content);

Expand Down
2 changes: 2 additions & 0 deletions src/StimulusBundle/tests/AssetMapper/ControllersMapGeneratorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ public function testGetControllersMap()
$logicalPath = 'fake-vendor/ux-package1/package-controller-second.js';
} elseif (str_ends_with($path, 'package-hello-controller.js')) {
$logicalPath = 'fake-vendor/ux-package2/package-hello-controller.js';
} elseif (str_ends_with($path, 'other-controller.ts')) {
return null;
} else {
// replace windows slashes
$path = str_replace('\\', '/', $path);
Expand Down
7 changes: 7 additions & 0 deletions src/StimulusBundle/tests/fixtures/assets/more-controllers/other-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// other-controller.js
// @ts-ignore
import { Controller } from '@hotwired/stimulus';

/* stimulusFetch: 'lazy' */
export default class extends Controller {
}
Loading