MR looks good.
Maybe a better name for the twig function would be something like `vite_get_chunk_path` it would better describe what it does and would allow adding more vite related twig functions in the future if needed.
Also before merge some documentation with usage example in the README will be needed.
Created MR from @yannickoo patch. The changes seem to work fine and fix the problem.
Will commit and release today.
Also pushed some kernel tests to the main branch that check rewritten paths in the dev and dist modes, to hopefully avoid similar problems in the future.
Code (added in
https://www.drupal.org/project/vite/issues/3448622
🐛
Asset paths for modules which names end with 'components' are rewritten incorrectly
Fixed
) for removing potential double slashes in components assets paths was replacing slashes also in https://... resulting in https:/.... which was treated as an absolute path. For some reason it didn't break when using the default localhost vite dev server url which is why it wasn't caught earlier.
Fix in https://git.drupalcode.org/project/vite/-/merge_requests/16 makes sure only double slashes at the beginning of asset paths are deduped and the dev server url is not modified.
Fixed in https://www.drupal.org/project/vite/issues/3444001 🐛 Styles imported in a chunk not being added to library RTBC which was released in 1.2.0 → .
As far as I'm aware, the current Vite version works fine with scss entrypoints.
Example:
# theme.libraries.yml
global-styling:
vite: true
css:
theme:
scss/style.scss: {}
// vite.config.js
import { defineConfig } from "vite";
import multiInput from "rollup-plugin-multi-input";
export default defineConfig(({ mode }) => {
return {
plugins: [multiInput()],
build: {
manifest: true,
rollupOptions: {
input: ["scss/**/*.scss", "!scss/**/_*.scss"],
},
},
};
});
It builds fine and produces vite manifest that is correctly handled by drupal/vite module:
// dist/.vite/manifest.json
{
...
"scss/style.scss": {
"file": "assets/scss/style-DYjrZ8Ot.css",
"src": "scss/style.scss",
"isEntry": true
},
...
}
@mvonfrie
The import multiInput from "rollup-plugin-multi-input" and then loading the plugin with multiInput.default() no longer works for me> I'm getting the following error:
You must supply options.input to rollup
Do you have at least one source file that matches paths from `build.rollupOptions.input`? I once encountered that error when trying to run build without actually having any files to build, after creating at least one, even empty, file that matches configured paths everything worked fine.
There doesn't currently seem to be a config option for that. Both hidding toolbar and linking logo to drupal.org is hardcoded.
It however can be fixed with a bit of custom code. Logo link is hardcoded in the page--mercury-editor template that can be overriden and showing admin toolbar is controlled by the `_hide_admin_toolbar` route option that can be altered → . I'm using following route subscriber to do that:
class MercuryEditorRouteSubscriber extends RouteSubscriberBase {
/**
* {@inheritdoc}
*/
protected function alterRoutes(RouteCollection $collection): void {
// Show admin navigation on Mercury Editor route.
$mercuryEditorRoute = $collection->get('mercury_editor.editor');
if ($mercuryEditorRoute !== NULL) {
$mercuryEditorRoute->setOption('_hide_admin_toolbar', FALSE);
}
}
}
It works quite well for the
new admin navigation →
(combined with changes from
https://www.drupal.org/project/mercury_editor/issues/3449328
✨
Hide new drupal admin navigation on mercury editor routes
Needs review
), not sure how it will look with other admin toolbars but it should make them show up.
Created merge request that adds support for removing the new admin navigation for the routes with the _hide_admin_toolbar option set to true, simillary to how other toolbars are removed.
With this change Mercury Editor page renders corectly without admin navigation toolbars:
Marking as RTBC. Changes from https://git.drupalcode.org/project/vite/-/merge_requests/12 worked for us in internal and client projects without any problems.
Changing to needs work as it is missing documentation of the new devDependencies option.
Change looks good. It seems that treating css as chunks worked in earlier Vite versions but was never the intended approach. https://vitejs.dev/guide/migration.html#corresponding-css-files-are-not-...
Give credit to developer who worked on the module.
Give credit to developer who worked on implementing module functionality.
Credit developers who worked on initial module implementation.
Local tasks (tabs) are by default cached per route, user.permissions and specific entity, for example test project and crop plan I was testing this changes have following CacheableMetadata
Using hook_menu_local_tasks_alter to remove tab under specific conditions is safe and there's even an example in drupal core https://git.drupalcode.org/project/drupal/-/blob/820d76b2c71d22132796969....
But I think you're right that replacing _entity_bundles restriction with bundle key in route param would be in this case a better approach. Updated the MR to use this approach, it seems to work well.
Created merge request with hook_menu_local_tasks_alter implementation that removes project logs tab for all route matches that don't have project plan in params. This results in 'Logs' tab being visible only on project plans and not on other plan types.
A quick fix could be to just check in the custom install task if $install_state['parameters']['existing_config'] is true and if so skip automatic installation of base modules.
Merged and released in v1.2.2 → .
Merged and released: https://www.drupal.org/project/payu_donations/releases/1.2.0 →
This is caused by a change in Vite 5. https://vitejs.dev/guide/migration.html#corresponding-css-files-are-not-...
In earlier versions css files were added as separate chunks, in Vite 5 for js entrypoints they are only listed in the js chunk under css key.
Manifest generated for a js file that imports css file:
- on Vite 3 and 4
{ "src/main.css": { "file": "assets/main-98fde70f.css", "src": "src/main.css" }, "src/main.js": { "css": [ "assets/main-98fde70f.css" ], "file": "assets/main-690fc99d.js", "isEntry": true, "src": "src/main.js" } } - on Vite 5
{ "src/main.js": { "css": [ "assets/main-gINCZU8i.css" ], "file": "assets/main-6jBz11gc.js", "isEntry": true, "src": "src/main.js" } }
Since we already support both Vite 3-4 and Vite 5 in other places (manifest location, dev server response codes) we should also ensure support for both versions in this case.
Change looks good, will merge it.
But first, I think it would be also good to provide an option to enable vite in all components/libraries of the theme/module right in the theme/module, so that vite configuration is contained in the theme/module it applies to and changes to settings.php aren't required for the theme/module to work.
Thinking of adding vite section to the .info.yml file that will allow enabling vite in all sdc components and/or all libraries defined in .libraries.yml, something like:
name: My theme
[...]
vite:
enableInAllComponents: true
enableInAllLibraries: true
[...]
MR !5248 adds vincentlanglet/twig-cs-fixer instead of friendsoftwig/twigcs about which is this issue.
That being said, friendsoftwig/twigcs maintainer is looking for someone to take over the maintainership and considers putting the project in the EOL status if no one will step up.
https://github.com/friendsoftwig/twigcs/issues/304
One alternative they point to is vincentlanglet/twig-cs-fixer (which MR !5248 adds).
Personally, I also prefer vincentlanglet/twig-cs-fixer since it also includes automatic fixes. We already using it for some time at work in Drupal based projects, and it works great.
Committed SDC integration in https://git.drupalcode.org/project/vite/-/commit/4fee955a75ceac335adfe82.... Instructions are in the README https://git.drupalcode.org/project/vite#sdc-integration.
Basically for SDC is required the same configuration as for normal theme/module libraries, only in libraryOverrides in component definition instead of in .libraries.yml.
Change looks good. The only thing to fix is one issue reported by phpstan (you can see it in the merge request ui).
The in_array function should be called with the third parameter set to true to strictly compare values, including checking their types. https://www.php.net/manual/en/function.in-array.php#refsect1-function.in...
Added props default value functionality implementation in https://git.drupalcode.org/project/drupal/-/merge_requests/5353. It passes test modified by justafish.
I implemented it in `sdc_additional_context` twig function that is executed in ComponentNodeVisitor and already used to add things to component context. The implementation loops through component props and if a given prop has a default value set and doesn't already have a value in the current context, the default value is added to the context (which makes it available in the component template).
The name of the new settings is rather generic but is only used for oEmbed. Should the name be changed to reflect that?
It is not used only for oEmbed. The primary use of `http_client_config` is in ClientFactory where it allows overriding the default http client settings, including the timeout (which by default is set to 30s).
The description in default.settings.php should probably also mention that changing this setting will impact all code that uses the http_client service without providing custom timeout explicitly.
Maybe a better way to approach this would be to provide a separate setting for the oEmbed ResourceFetcher timeout, since from what I understand the 5s limit was added in https://www.drupal.org/project/drupal/issues/3202145 → specifically to stop using the default global timeout value.
Merged and released in https://www.drupal.org/project/farm_map_custom_layers/releases/1.1.0 → .
Merged and released in https://www.drupal.org/project/farm_map_custom_layers/releases/1.1.0 → .
It would be nice to implement this in a general way to allow for including potential similar scripts for other frameworks.
Maybe we could add a concept of devDependencies to the vite section of library definition. It could allow providing a list of other libraries like in the default 'dependencies' section, but they would be attached only when running in the vite development mode.
Then for this specific use case, in this module we could define a vite/plugin-react-refresh library that includes the script. Then in a theme/module where vite-plugin-react is used, in the library definition this library could be added to the devDependencies:
react-block:
vite:
enabled: true
devDependencies:
- vite/plugin-react-refresh
js:
...
The plugin-react-refresh script will also need the current vite dev server url. Since it can be different for every theme/module passing it through drupalSettings won't really work because the script won't know which url to use. Maybe we could pass it to all devDependecies through data-vite-dev-server-url attribute then in scripts it could be fetched by accessing attributes of document.currentScript.
Added two last comments in the merge request, after resolving them this will be ready to be merged.
Added some comments in the merge request.
WMS also allows two optional parms `VERSION` and `STYLES`, some WMS services even require them to correctly load maps. To make the WMS support more complete, it would be nice to also add an option to provide these two params in the custom map layer configuration.
Related to
https://www.drupal.org/project/drupal/issues/3365480
🐛
[SDC] Improve error handling during prop validation errors
Fixed
where similar problem was fixed for passed props validation.
Same fix of adding `Constraint::CHECK_MODE_TYPE_CAST` as a third argument to `$this->validator->validate()` applied at https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/sdc/s... should help. It will validate schema using loose type checking https://github.com/justinrainbow/json-schema/blob/master/src/JsonSchema/... that will treat empty and associative php arrays as valid objects.
You already created one issue for this project with exactly the same content and patch.
I replied in the
original issue
🐛
All dependencies must be prefixed with the project name, for example "drupal:"
Closed: works as designed
why omitting project name is in this case intentional workaround and won't be fixed:
https://www.drupal.org/project/farm_map_custom_layers/issues/3367910#com...
🐛
All dependencies must be prefixed with the project name, for example "drupal:"
Closed: works as designed
As stated in the comment right above the dependency declaration, omitting the project name is an intentional workaround for how packages.drupal.org doesn't handle declaring a dependency on a module from a distribution very well. As seen in
https://www.drupal.org/project/farm_forest/issues/3190919 →
this is a workaround suggested by one of the maintainers of
packages.drupal.org →
.
Also, if packages.drupal.org would handle it right, the dependency with project id would have to be `farm:farm_map`, the standalone
farm_map project →
is no longer maintained, and the version updated to farmOS v2 (D8+) is now included directly in the
farmOS →
distribution.
Configuring domain solves the problem of binding ports directly, but it is still an additional configuration step that could be skipped by proxying requests to vite dev server. Also, when running multiple vite dev servers simultaneously inside single project (for example in a multisite setup for common base theme and specific site theme or maybe for a theme and a decoupled react based block) you would need to configure multiple domains pointing to different ports.
One possible approach to allow not using the module in production while retaining all the advantages of integration with vite manifest.json could be to add an optional build step to `.libraries.yml`. So for development you would do everything the same as now, but when you want to deploy changes to production you would build assets using vite and then execute a build step for `.libraries.yml` that would output rewritten libraries (which is done by default in production) back to the `.libaries.yml` file.
It seems that these changes:
- require manifest.json to be generated for dev mode,
- require vite to be configured to not use hashes in built file names,
- with the module disabled in production (which is the motivation for these changes) it requires to manually set all built chunks/dependencies declared in manifest.json directly in .libraries.yml.
Overall these changes break current functionality of the module, require specific vite configuration and require manual .libraries.yml modifications that currently are done automatically.
I don't see a way to merge this as a default behavior, it would need to be behind some setting/flag that needs to be enabled to switch to proposed behavior. Some documentation that would list caveats/requirements to work in this mode should also be added before possible merge.
Also running phpcs and phpstan with configurations provided in the project repo results in the following new errors:
www-data@172775eb8550:~/drupal/web/modules/contrib/vite$ vendor/bin/phpcs .
FILE: /var/www/drupal/web/modules/contrib/vite/src/Manifest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
128 | ERROR | [x] Expected 1 space after FUNCTION keyword; 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------
FILE: /var/www/drupal/web/modules/contrib/vite/src/Vite.php
----------------------------------------------------------------------------------------------------------------------------
FOUND 13 ERRORS AFFECTING 13 LINES
-----------------------------------------------------------------------------------------------------------------------------
195 | ERROR | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
203 | ERROR | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
220 | ERROR | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
228 | ERROR | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
251 | ERROR | [ ] Missing short description in doc comment
252 | ERROR | [ ] Missing parameter comment
253 | ERROR | [ ] Missing parameter comment
254 | ERROR | [ ] Missing parameter comment
255 | ERROR | [ ] Description for the @return value is missing
257 | ERROR | [x] There must not be a space before the colon in a return type declaration
258 | ERROR | [x] Opening brace should be on the same line as the declaration
262 | ERROR | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
267 | ERROR | [x] Expected newline after closing brace
-----------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 8 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------------------------------------------------
Time: 99ms; Memory: 12MB
www-data@172775eb8550:~/drupal/web/modules/contrib/vite$ vendor/bin/phpstan analyse .
Note: Using configuration file /var/www/drupal/web/modules/contrib/vite/phpstan.neon.
7/7 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
------ ------------------------------------------------------------------------------------------------------------
Line src/Vite.php
------ ------------------------------------------------------------------------------------------------------------
257 Method Drupal\vite\Vite::getNewLibraryPath() never returns null so it can be removed from the return type.
267 Else branch is unreachable because previous condition is always true.
------ ------------------------------------------------------------------------------------------------------------
No, didn't yet get to preparing them. The starting point would depend on what you want to do. Use vite for building theme styles/scripts? Maybe for a custom decoupled block? Configuration for integration with vite dev server will also depend (at least until https://www.drupal.org/project/vite/issues/3340765 ✨ Proxy requests to vite dev server Active is implemented) on your development environment setup.
On GitHub there seems to be a project that uses this module for a custom theme with styles written in scss https://github.com/bronisMateusz/visualize, maybe it can be of some help.
I also created a new issue https://www.drupal.org/project/vite/issues/3349311 📌 Example configurations/starting points Active to track creating some examples. If you have some specific use case/configuration you would want to see an example of, you can describe it there, and I will try to implement it on the weekend.
The module has two primary purposes:
- integration with vite dev server for development,
- and integration with vite asset manifest for production.
The proposed resolution would only work if assets built by vite would have static names, which is by default not the case (they have dynamically generated hashes appended to the filenames).
The module implements vite backend integration as described on https://vitejs.dev/guide/backend-integration.html#backend-integration.
I don't plan to explore the requested use case. That said, I'm open to accept contributions If someone would want to implement it.
Even if we will stick with Drupal PostgreSQL minimum version requirements, it could be nice to add automatic pg_trgm extension installation when PostgreSQL v13 or newer is detected.