-
-
Notifications
You must be signed in to change notification settings - Fork 192
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.
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
[AssetMapper] Allow to define entrypoint in importmap.php #1026
base: 2.x
Are you sure you want to change the base?
Conversation
relate to symfony/symfony#53912 |
src/PackageJsonSynchronizer.php
Outdated
@@ -267,8 +268,14 @@ private function updateImportMap(array $importMapEntries): void | |||
$this->io->writeError(sprintf('Updating package <comment>%s</> from <info>%s</> to <info>%s</>.', $name, $version, $versionConstraint)); | |||
} | |||
|
|||
$arguments = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A package with version cannot be an entrypoint... so entrypoints should not be concerned here, no ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, i see what you mean.
If i remember correctly, it was to avoid too many if
levels but I can add it inside the following condition
Do we allow bundle to change the public/index.php file? Or to create new webpack configs ? I really find weird that a bundle could install things like this. And a bit dangerous. And still wait for a use case that require to create a entrypoint .... maybe you could describe yours for the package in question ? That really could help me understand the need. |
src/PackageJsonSynchronizer.php
Outdated
@@ -147,7 +147,7 @@ private function resolveImportMapPackages($phpPackage): array | |||
} | |||
|
|||
$dependencies = []; | |||
|
|||
$entrypoints = $packageJson->read()['symfony']['entrypoints'] ?? []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took some time to refresh my memory and re-read all this code 😅
So .. nope : this is not related to importmap entrypoints, but to stimulus bundle + controllers.json file. Thus should not be used here :)
Should not be used here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a pure technical level, we may want to use another key, and definitively one nested under the importmap
key.
The [symfony][entrypoints] is used by flex and is or has been used by stimulus bundle / stimulus bridge / webpack encore bundle.
flex/src/PackageJsonSynchronizer.php
Lines 365 to 370 in 1418926
foreach ($packageJson->read()['symfony']['entrypoints'] ?? [] as $entrypoint => $filename) { | |
if (!isset($newControllersJson['entrypoints'][$entrypoint])) { | |
$newControllersJson['entrypoints'][$entrypoint] = $filename; | |
} | |
} | |
} |
Meaning it would have different meaning, and behaviour
, wether someone uses Webpack or AssetMapper.
Maybe there is something to do in the symfony[importmap][ PACKAGE_NAME ] key.
Currently we allow here two shapes
$importMapName => [
'package' => 'foobar',
'version' => 'foobar',
]
and
$importmapName => 'path:foobar'
As seen in my other comment, the first one is out of scope (entrypoints cannot have version).
But I suppose you could here allow another array syntax, like
$importMapName => [
'package' => 'path:foobarr',
'entrypoiit' => true,
]
Or something like replace path:
with entrypoint:
$importMapName => 'entrypoint:foobar',
(that would give the path and specifu an entrypointà)
src/PackageJsonSynchronizer.php
Outdated
$dependencies[$importMapName] = [ | ||
'path' => $path, | ||
'entrypoint' => \in_array($importMapName, $entrypoints, true), | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(update) collapsed my comment here: it was not constructive
By definition, a dependency cannot be an importmap entrypoint :)
[TL;DR] This feature is cool, we need it in flex. As it or in other way, but it would be useful to describe a bundle js dependency as an entry point. Hello @smnandre, I was concern by the topic "Asset Mapper with Bundle" because I work on Sylius applications, and we use themes (drove by the Sylius Theme Bundle). The idea behind was to distribute Sylius Theme as Symfony Bundle / Sylius Plugin (that's the same thing) to offer the ability for a developer to make a A Sylius theme package / bundle / plugin it's almost templates (twig files), CSS and JS. Since we have introduced Asset Mapper in our Sylius project, I encountered all the problems describe in this looong topic. Basically: how to smoothly install a bundle with JS and CSS dependencies in a Symfony project where assets are Asset Mapper drove. I'm not going to rehash the game and I understand both side arguments. I resolve a lot of my problems using the same methods as Symfony UX packages: add {
"name": "my-vendor/my-beautiful-theme",
"description": "Beautiful Theme for Sylius",
"license": "MIT",
"version": "1.0.0",
"symfony": {
"importmap": {
"alpinejs": "3.13.2",
"flickity": "3.0.0",
"my-vendor/my-beautiful-theme": "path:%PACKAGE%/my-entrypoint.js"
}
}
} I use the idea describe in this comment to declare theme assets folder as Asset Mapper path. (Part 2: How do I expose my bundle's own assets to a consumer of the bundle?) And it's work like a charm. 🎉 But now there is my point: theme bundles comes with the whole Sylius app templates. Whole app means that somewhere we use // Automatically added by flex FTW but it miss that this is an entrypoint
'my-vendor/my-beautiful-theme' => [
'path' => './vendor/my-vendor/my-beautiful-theme/assets/my-entrypoint.js'
], to '@monsieurbiz/master-theme' => [
'path' => './vendor/my-vendor/my-beautiful-theme/assets/my-entrypoint.js',
'entrypoint' => true,
], in the That's why I agree with @Jibbarth: It woul be very very cool to have this feature! |
Well, once more.... we're not talking about "bundle assets" but Because assets from your bundle are exposed via both asset package and AssetMapper. I have no power to prevent you for moving on this topic, and no intention to do so... Meaning this should, i think, be a new configuration key somewhere.. |
Hi @smnandre, To be honest, I'm not sure to understand your reply because it seems not related to my post. You mention To be clear:
This process exists and works perfectly. No hack, no trick, no billions of external packages. This is how Symfony UX bundle works too. The only purpose of this PR, and my only point is: let us the capability to precise that one or more bundle JS local files are entry points in I you
If the answer is yes, you probably not use Symfony UX and you are probably not the right person to deal with our point about entrypoint. If the answer is no, you have to understand that this small feature could be a game changer for peoples who work on third party bundle with app sized JS file (theme bundle, full admin bundle, dashboard bundle, ....) Move from actual {
"name": "my-vendor/my-beautiful-theme",
"description": "Beautiful Theme for Sylius",
"license": "MIT",
"version": "1.0.0",
"symfony": {
"importmap": {
"alpinejs": "3.13.2",
"flickity": "3.0.0",
"my-vendor/my-beautiful-theme": "path:%PACKAGE%/my-entrypoint.js"
}
}
} to {
"name": "my-vendor/my-beautiful-theme",
"description": "Beautiful Theme for Sylius",
"license": "MIT",
"version": "1.0.0",
"symfony": {
"entrypoints": ["my-vendor/my-beautiful-theme"]
"importmap": {
"alpinejs": "3.13.2",
"flickity": "3.0.0",
"my-vendor/my-beautiful-theme": "path:%PACKAGE%/my-entrypoint.js"
}
}
} for a result from actual // Automatically added by flex FTW but it miss that this is an entrypoint
'my-vendor/my-beautiful-theme' => [
'path' => './vendor/my-vendor/my-beautiful-theme/assets/my-entrypoint.js'
], to '@monsieurbiz/master-theme' => [
'path' => './vendor/my-vendor/my-beautiful-theme/assets/my-entrypoint.js',
'entrypoint' => true,
], should not create such a debate to my mind. But, I know you have to address tons of different problems for the Symfony community, and that's a huge and time-consuming work. I'm open to try to answer to your questions and ready to try to calm your fears for this point. Cheers |
Hi @lanfisis! The general asset situation is currently a bit of a mess, and I can assure you I've been once or twice using Symfony UX packages :) I'm saying again and again bundles can expose their assets, so it's only a question about external dependencies and automatic installation. And i will precise this everytime it's needed, I don't want anyone to believe this is not the case currently.
This is why I'm telling you I won't nor want (nor can) oppose this feature. And I continue to answer on the technicalities and consequences on some of the suggested implementations (see my last comment on this matter. symfony/symfony#53912 (comment)) |
Thanks for the quick answer (as usual).
I was no doubt about this, my question was more rhetoric than really relevant. In a previous message, you said:
But this is a flex feature implemented 2 years ago by Ryan and Kevin, and used by a lot of official bundles. Almost all the UX bundle add external dependencies to
That's not true. It is used to manage any bundle located js file as you can see here. There is absolutely nothing in the code that constraint the use of this feature for Stimulus controller.
I hope I have to show you at least one example of why we need this feature.
Nope, you are the only one who deal with this topic at the beginning. Adding external dependencies and / or JS bundle files into the |
There is not in this file any Symfony entrypoint key. When i comment something specific about the code, it is only about the code. No thing more . —- You keep comparing to multiple UX packages. But you must have seen none of them add a new entrypoint. Which is specifically what you say you want here. So yeah i’m not getting the argument here. —- Flex is not managed by the UX Core Team, nor is AssetMapper. The StimulusBundle is, but still follows the BC promise of Symfony in general. —- The fact you seem to ignore or let aside the difference between entrypoints and other dependencies proves me the entire system must be re-documented and/or clarified, probably changed too. — On the other PR I explain certain things about technical points and try to separate the very différent things asked related to bundles. — You wont convince me and this is not a problem and should not be one. One last time: I cannot and will not block this feature. I can point some technicalities to avoid messing with users that never signed for it. But allow me to not be celebrating it either. |
Ok, so the key is probably not good named. I agree with this. And this comment is surely relevant in the scope of this PR.
I use UX bundle as example because this is the only way to add dependencies on My point is: UX packages have to add JS dependencies on Peoples of the community said: in the context certain kind of bundle, as I describe and I can describe again if needed, we want to do exacltly the same thing but we want to precise that one of this package is an entrypoint because the bundle comes with templates that include this entrypoint => we get comments about the fact that's "weird" and that there is no concrete application. Let's assume "that" is weird to me.
Be sure that I really understand what an entry point is. I'm working on the ability to make Sylius Theme bundle distributable with an asset mapper compatibility for a year. Because there's no documentation, it takes me hours to find how to smoothly, backed by official Symfony tool and trying to avoid dirty fixes, make this working by reading codes source of Asset Mapper, Flex, UX bundles, .... So you're right: this need to be documented and clarified. But this is a group work that have to be backed by the advices and the need of the entire Symfony community, not your own personal opinion.
You said "I'm saying again and again bundles can expose their assets ..." and we (@Jibbarth and I) just add "... with the capability to precise that one of this asset is an entry point". Sorry, but it's to little drive me crazy to see you give your negative point of you like "I really find weird that a bundle could install things like this. And a bit dangerous." or " But allow me to not be celebrating it either." on PR like this where people have takes time to produce code, are aware about technical observation and describe real life use case. This is only based on your guts feeling too me, no based on fact. But, agree to disagree about that an asset bundle could be or not an entrypoint. I hope that @Jibbarth will takes time to change his code according to your needs and I hope that this feature will be available on flex soon because it's a real life needs. Have a nice day. |
@lanfisis reached out to propose a private discussion to address and smooth out differences in perspective and clarify a few points. I gladly accept (we’ll aim to do this as soon as possible) and truly appreciate the positive and constructive approach. |
src/PackageJsonSynchronizer.php
Outdated
if (isset($importMapEntry['entrypoint']) && true === $importMapEntry['entrypoint']) { | ||
$arguments[] = '--entrypoint'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (isset($importMapEntry['entrypoint']) && true === $importMapEntry['entrypoint']) { | |
$arguments[] = '--entrypoint'; | |
} | |
if (isset($importMapEntry['entrypoint']) && true === $importMapEntry['entrypoint']) { | |
$arguments[] = '--entrypoint'; | |
} |
This must be updated (moved in the code -- see below) as it will potentially generate an incorrect state.
Entrypoints cannot be remote packages.
In ImportMapGenerator
if ($rootImportEntries->get($entryName)->isRemotePackage()) {
throw new \InvalidArgumentException(\sprintf('The entrypoint "%s" is a remote package and cannot be used as an entrypoint.', $entryName));
}
Remote package = package with a version
In ImportMapEntry.php
public function isRemotePackage(): bool
{
return null !== $this->version;
}
Version and path are exclusive properties
In ImportMapConfigReader.php
if (isset($data['version'])) {
throw new RuntimeException(\sprintf('The importmap entry "%s" cannot have both a "path" and "version" option.', $importName));
}
Package have either version or path
In ImportMapConfigReader.php
if (null === $version) {
throw new RuntimeException(\sprintf('The importmap entry "%s" must have either a "path" or "version" option.', $importName));
}
Tip
So entrypoints must be local. And this must be moved line 277
(inside the if (isset($importMapEntry['path'])) {
)
Another question: should we allow overriding existing user entrypoints? Do we want to allow that ?
Now, it allow "@mybundle/script.js": "entrypoint:%PACKAGE%/script.js" or "@mybundle/script.js": {"version": "entrypoint:%PACKAGE%/script.js"} or "@mybundle/script.js": {"version": "path:%PACKAGE%/script.js", "entrypoint": true}
Let's sum up. Currently, we can declare assets for asset-mapper with following ways in package.json {
"symfony": {
"importmap": {
"jquery": "^3.7.1", // Remote
"myjquery": { // custom name
"version":"^3.7.1",
"package": "jquery"
},
"@mybundle/script.js": {
"version": "path:%PACKAGE%/script.js",
},
"@mybundle/script2.js": "path:%PACKAGE%/script2.js"
}
}
} To be able to use one of these as entrypoint, as proposed by @smnandre, I added following possibilies {
"symfony": {
"importmap": {
"@mybundle/script.js": {
"version": "path:%PACKAGE%/script.js",
"entrypoint": true,
},
"@mybundle/script2.js": "entrypoint:%PACKAGE%/script2.js",
"@mybundle/script3.js": {
"version": "entrypoint:%PACKAGE%/script3.js"
}
}
}
} I guess, for a while, third-party bundle should use the first way
because it will not throw any error while requiring the asset for app that don't have latest flex version, but user still have to add the "entrypoint => true" in their importmap.php. |
$package = $constraintConfig['package'] ?? $importMapName; | ||
} else { | ||
if (\is_string($constraintConfig)) { | ||
// Case "jquery": "^3.0" | "@mybundle/script.js": "path:%PACKAGE%/script.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can clean this comment if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure to read it correctly to be fair 😅
Seems ok! |
Ok to me too ! I've personally adopted another way to add entrypoints after some talks with @smnandre, this small feature could be useful and have no side effects I think. |
Hello 👋
I wanted that one of my package to be able to define an asset as entrypoint inside importmap.php to be allow call
importmap('@mypackage/entry.js')
inside a template of the package.I made a naive modification, not sure if it was the purpose of the
[symfony][entrypoints]
node of the package.json.Maybe it would be fit directly inside the
[symfony][importmap]
node.WDYT ?
UPDATE : see latest proposal here #1026 (comment)