-
Notifications
You must be signed in to change notification settings - Fork 24
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
Vite plugin for FOSJsRoutingBundle #24
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for cosmic-bubblegum-aa631a ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
I have used some hooks for rollup that are not inside Vite's Plugin type. So I casted it to Plugin, please advise if there is an alternative or if I didn't use the right hooks. TODO:
P.S. This is my first Vite plugin. |
Hi @SkipTheDragon, some ideas and recommendations :
Good luck for the future... ps : maybe as a high priority look for prettier |
…ync, writeFileSync bellow)
Merge Playground to featbranch
…s frequently than before feat: more verbose messages
Hi, thanks for the playground and feedback! I have already solved some of the raised concerns, hopefully I will have everything ready by the end of next week. As a status update for this PR, the following are WIP:
|
fix: changed docs to match feat: configured and tested the playground.
fix: tests
I'm not quite sure on how to solve the pnpm-lock.yaml conflict, but I can look into it if needed. |
hi @SkipTheDragon , thank you for your contribution and sorry for my late response. given the amount of code I haven't yet taken the time to do my review. but your contribution is very appreciated and as soon as I have a little time I will take care of doing the review. have a nice week end ! |
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.
hi @SkipTheDragon !
I finally took the time to do the code review. sorry again for the delay... hope you don't mind diving back into the code
First, thank you very much for your great work 🙏🙏.
from a general point of view the whole thing holds together very well and that's the main thing! so I left you 2 - 3 comments here and there. afterwards I will have to look in more depth at the transform
function of the file src/vite-plugin-symfony/src/fos-routing/index.ts
I admit that given the lateness of the hour I did not had the courage to tackle the regular expression 🤯🤯
good night !!
last thing. this crashes in my code import Routing from "fos-router"; it does not recognize the default import. I suspect a problem between the es module and the umd format for local packages. but I don't fully understand the subject. |
hi @SkipTheDragon, |
ok I found the solution, it's because the 'fos-router' package is installed from local system and it's a commonjs package. explanation Automatic dependency discovery solution: import { defineConfig } from "vite";
import symfonyPlugin from "vite-plugin-symfony";
export default defineConfig({
plugins: [symfonyPlugin()],
// that is for vite dev server
optimizeDeps: {
include: ["fos-router"],
},
build: {
rollupOptions: {
input: {
app: "./assets/app.js",
},
},
// that is for the build step
// for the @rollup/plugin-commonjs
commonjsOptions: {
include: [
"vendor/friendsofsymfony/jsrouting-bundle/Resources/public/js/router.js",
/node_modules/,
],
},
},
}); another solution would be to ask FOSJsRoutingBundle to export a ESModule compatible export of their package (just a transpiled version of their router.ts would be enough ?). // https://github.com/FriendsOfSymfony/FOSJsRoutingBundle/blob/master/Resources/package.json
{
"name": "fos-router",
"version": "2.5.0",
"main": "public/js/router.js",
"module": "public/js/router.mjs",
...
} or to maintain an up to date version of their package in npmjs cf your comment |
Hi, thanks for the thorough review, I'll see what I can do. :) |
I have added this in the docs for now, until they add it on NPM or I find some time to add it myself. |
*/ | ||
const defaultFosRouterPluginOptions = { | ||
args: { | ||
target: "var/cache/fosRoutes.json", |
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 am bothered by this target
which is by default in the server.watch.ignored
(src/vite-plugin-symfony/src/entrypoints/index.ts
) we could add an exception this poses us 2 problems.
- the 2 plugins are dependent on each other at the config level
- if a developer defines the
server.watch.ignored
option by himself, he will not think to pay attention to thefosRouting.json
file.
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.
Hi @SkipTheDragon,
We have one last blocker left with this
what do you think about target path ?
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 don't really understand the problem, the target does not have anything to do with server.watch.ignored.
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.
However, you have written a case with the target here : https://github.com/SkipTheDragon/symfony-vite-dev/blob/aeb00940877ea7eb9d335565ecf48c35c288f385/src/vite-plugin-symfony/src/fos-routing/index.ts#L106
so you wanted to listen for changes in the json file to reload the page ?
currently this scenario cannot occur
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 am not quite sure how to solve this, and my time has been pretty limited these past months, I don't know when i will be able to look deeper into this.
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.
no problem, I will see and I will offer you a solution.
like you I haven't spent much time on this project in recent weeks but it would be cool if we could merge this pr.
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.
Hi @SkipTheDragon,
I applied a small review last night that I applied on the main branch (in the meantime I had switched TypeScript to strict mode which caused some errors)
https://github.com/lhapaipai/symfony-vite-dev/tree/skipthedragon/fos-routing
I left you some comments on some blockers that I have and that require your approval to continue and I will correct this on the code then I will translate the doc into fr.
I think I will merge your code for the next major release in a few days.
It will be great there will be lots of new features !!
Thank you !! and have a nice day
No description provided.