New version of prettier fails on ToolbarButton because of (I guess) what it thinks is a reserver keyword. [error] koha-tmpl/intranet-tmpl/prog/js/vue/components/ToolbarButton.vue: SyntaxError: Unexpected token, expected "{" (1:6) [error] > 1 | class [error] | ^ The error does not really make sense, the code is: class: { type: String, default: "btn btn-default", }, A solution could be to rename the property, but actually we never use another value than the default one. The easiest seems to simply remove this property.
% prettier --version 3.4.2
Created attachment 176292 [details] [review] Bug 38854: Remove 'class' property from ToolbarButton New version of prettier fails on ToolbarButton because of (I guess) what it thinks is a reserver keyword. [error] koha-tmpl/intranet-tmpl/prog/js/vue/components/ToolbarButton.vue: SyntaxError: Unexpected token, expected "{" (1:6) [error] > 1 | class [error] | ^ The error does not really make sense, the code is: class: { type: String, default: "btn btn-default", }, A solution could be to rename the property, but actually we never use another value than the default one. The easiest seems to simply remove this property. % prettier --version 3.4.2 Test plan: Upgrade prettier (yarn add prettier), confirm you have a more recent version Try to tidy the file using `prettier koha-tmpl/intranet-tmpl/prog/js/vue/components/ToolbarButton.vue` Without this patch you get the "Unexpected token" error. With this patch apply you don't Also rebuild the dist files using `yarn js:build` and confirm that the buttons in the toolbar of the ERM and Preservation modules still appears and are styled like before.
Hi Jonathan, After following the test plan, I wasn't able to recreate the bug. Specifically, even with the latest version of prettier (3.4.2) downloaded and even without the patch applied, there was no "SyntaxError: Unexpected token". Note that I am on a Windows 11 machine running WSL with an Ubuntu distribution. My development environment includes Docker Desktop for containerization, and the Koha testing environment is running inside WSL. Leo
I attempted to test, but don't get the error before the patch. 1. I ran: yarn add prettier yarn add v1.22.22 [1/4] Resolving packages... [2/4] Fetching packages... [3/4] Linking dependencies... warning "@redocly/cli > redoc > @cfaester/enzyme-adapter-react-18@0.8.0" has unmet peer dependency "enzyme@^3.11.0". warning " > eslint-config-prettier@9.1.0" has unmet peer dependency "eslint@>=7.0.0". warning " > eslint-plugin-prettier@5.1.3" has unmet peer dependency "eslint@>=8.0.0". warning "swagger-cli > @apidevtools/swagger-cli > @apidevtools/swagger-parser@10.1.0" has unmet peer dependency "openapi-types@>=7". [4/4] Building fresh packages... success Saved lockfile. success Saved 1 new dependency. info Direct dependencies └─ prettier@3.4.2 info All dependencies └─ prettier@3.4.2 Done in 58.41s. 2. Prettier version after update: prettier --version 3.4.2 3. Output: prettier koha-tmpl/intranet-tmpl/prog/js/vue/components/ToolbarButton.vue <template> <router-link :to="to" :class="class" ><font-awesome-icon v-if="icon" :icon="icon" /> {{ title }}</router-link > </template> <script> export default { props: { to: { type: [String, Object], }, class: { type: String, default: "btn btn-default", }, icon: { type: String, required: false, }, title: { type: String, }, }, name: "Toolbar", } </script>
Same as the above - no errors. Are you using a prettier.config.js file in your branch with any additional config extending the defaults?
Or made any further changes in .prettierrc.js?
(In reply to Matt Blenkinsop from comment #6) > Or made any further changes in .prettierrc.js? Good catch! I indeed had an incorrectly-positioned config entry in .prettierrc.js (context is bug 38664). This is invalid, sorry for the noise!
The patch can still be useful as it removes an unused property. What do you think Matt? Keeping for flexibility or removing to keep the code clean? It's also confusing to pass a value when it's actually the default value.
I think we can remove it - if it needs adding back in at any point then we can make sure to use terminology that doesn't conflict with reserved keywords
(In reply to Matt Blenkinsop from comment #9) > I think we can remove it - if it needs adding back in at any point then we > can make sure to use terminology that doesn't conflict with reserved keywords ok thanks for confirming. I have adjusted the bug title.
Created attachment 176450 [details] [review] Bug 38854: Remove 'class' property from ToolbarButton We never use another value than the default one. We can simply remove this property. Test plan: Rebuild the dist files using `yarn js:build` and confirm that the buttons in the toolbar of the ERM and Preservation modules still appears and are styled like before.
Created attachment 176457 [details] [review] Bug 38854: Remove 'class' property from ToolbarButton We never use another value than the default one. We can simply remove this property. Test plan: Rebuild the dist files using `yarn js:build` and confirm that the buttons in the toolbar of the ERM and Preservation modules still appears and are styled like before. Signed-off-by: William Lavoie <william.lavoie@inLibro.com>
Looks like there are two errors: 1) AgreementsShow the class has been removed from an <a> tag that isn't part of the ToolbarButton component. The Delete button is styled incorrectly as a result 2) TrainsShow - the ToolbarButton component for editing trains has now got a '_class' property instead of having the 'class' removed (lines 93-101)
Created attachment 176903 [details] [review] Bug 38854: Revert incorrect changes
(In reply to Matt Blenkinsop from comment #13) > Looks like there are two errors: > 1) AgreementsShow the class has been removed from an <a> tag that isn't part > of the ToolbarButton component. The Delete button is styled incorrectly as a > result Oops, good catch! > 2) TrainsShow - the ToolbarButton component for editing trains has now got a > '_class' property instead of having the 'class' removed (lines 93-101) Oops again, that was part of the first attempt. Thanks! I've added a follow-up patch to help the review, but please squash them when QAing.
Created attachment 176971 [details] [review] Bug 38854: Remove 'class' property from ToolbarButton We never use another value than the default one. We can simply remove this property. Test plan: Rebuild the dist files using `yarn js:build` and confirm that the buttons in the toolbar of the ERM and Preservation modules still appears and are styled like before. Signed-off-by: William Lavoie <william.lavoie@inLibro.com> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Squashed and PQA
This no longer applies, can you please check? Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 38854: Revert incorrect changes error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/AgreementsShow.vue). error: could not build fake ancestor Patch failed at 0001 Bug 38854: Revert incorrect changes hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-38854-Revert-incorrect-changes-cicgpxvi.patch
Sorry, forgot to obsolete the squashed patch
Try again, should be fine now
The dependecy is not even signed off yet :(
(In reply to Katrin Fischer from comment #21) > The dependecy is not even signed off yet :( Obv wrong bug, please ignore.
Pushed for 25.05! Well done everyone, thank you!
Part of the work done here, plus abstracting the component(s) to prevent things like these from being needed (change the same thing everywhere) has already been done in bug 37301 for many months. Any chance we can get some eyes on that bug?