You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@jena.apache.org by "kinow (via GitHub)" <gi...@apache.org> on 2024/10/12 15:16:10 UTC
[PR] Gh 2767 fix js warnings [jena]
kinow opened a new pull request, #2768:
URL: https://github.com/apache/jena/pull/2768
GitHub issue resolved #2767
Pull request Description:
- Fixes grammar/typos
- Fixes variables that are ignored but have a name (instead of using `_`)
- Fixes types that are used but not imported
- Fixes code that uses dependencies that are not declared in our `package.json`
This last point is the most important one. We had some code modifying `codemirror`, which comes via Zazuko libs, but we never declared in `package.json`, using only as transitive. Now it's listed in our code as we actually import that code and modify it for our Query component.
But the most important change was replacing [`query-string`](https://github.com/sindresorhus/query-string) by [`qs`](https://github.com/ljharb/qs). We had both, in different parts of the code, but luckily we had unit tests covering that, so replacing was really easy and doesn't seem to break anything.
Choosing between the two was not straightforward as none is maintained by a big community or known company. After some searching, and comparing usage & downloads, looks like qs has a larger user base, and since its description mentions "security", I thought it'd be OK to keep that one and drop `query-string` (which might still be used in Zazuko components... although looks like they also have `qs` somewhere in their other transitive dependencies -- JS dependencies are complicated :sighs:)
- https://npm-compare.com/qs,query-string
- https://npm-compare.com/qs,query-string,querystring,url-parse
I grouped each change into separate commits so that I can revert/drop any if needed. Let me know if these should be squashed before the merge, and I will do so :+1:
----
- [x] Tests are included.
- [ ] Documentation change and updates are provided for the [Apache Jena website](https://github.com/apache/jena-site/)
- [ ] Commits have been squashed to remove intermediate development commit messages.
- [x] Key commit messages start with the issue number (GH-xxxx)
By submitting this pull request, I acknowledge that I am making a contribution to the Apache Software Foundation under the terms and conditions of the [Contributor's Agreement](https://www.apache.org/licenses/contributor-agreements.html).
----
See the [Apache Jena "Contributing" guide](https://github.com/apache/jena/blob/main/CONTRIBUTING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] Gh 2767 Fix Fuseki UI JavaScript warnings [jena]
Posted by "afs (via GitHub)" <gi...@apache.org>.
afs merged PR #2768:
URL: https://github.com/apache/jena/pull/2768
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] Gh 2767 Fix Fuseki UI JavaScript warnings [jena]
Posted by "kinow (via GitHub)" <gi...@apache.org>.
kinow commented on code in PR #2768:
URL: https://github.com/apache/jena/pull/2768#discussion_r1797718743
##########
jena-fuseki2/jena-fuseki-ui/src/utils/query.js:
##########
@@ -49,6 +49,6 @@ export function createShareableLink (query, path) {
path +
'?' +
// Same as YASGUI does, good idea to avoid security problems...
- queryString.stringify({ query })
+ qs.stringify({ query })
Review Comment:
```
kinow@ranma:~/Development/java/workspace/jena/jena-fuseki2/jena-fuseki-ui$ yarn why query-string
yarn why v1.22.22
[1/4] Why do we have the module "query-string"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "query-string@6.14.1"
info Reasons this module exists
- "@zazuko#yasqe" depends on it
- Hoisted from "@zazuko#yasqe#query-string"
info Disk size without dependencies: "52KB"
info Disk size with unique dependencies: "120KB"
info Disk size with transitive dependencies: "120KB"
info Number of shared dependencies: 4
Done in 0.17s.
kinow@ranma:~/Development/java/workspace/jena/jena-fuseki2/jena-fuseki-ui$ yarn why qs
yarn why v1.22.22
[1/4] Why do we have the module "qs"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "qs@6.13.0"
info Reasons this module exists
- "express" depends on it
- Hoisted from "express#qs"
- Hoisted from "cypress#@cypress#request#qs"
- Hoisted from "express#body-parser#qs"
- Hoisted from "@zazuko#yasqe#superagent#qs"
- Hoisted from "@zazuko#yasqe#superagent#formidable#qs"
info Disk size without dependencies: "616KB"
info Disk size with unique dependencies: "740KB"
info Disk size with transitive dependencies: "1.66MB"
info Number of shared dependencies: 13
Done in 0.18s.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] Gh 2767 fix js warnings [jena]
Posted by "kinow (via GitHub)" <gi...@apache.org>.
kinow commented on PR #2768:
URL: https://github.com/apache/jena/pull/2768#issuecomment-2408598292
Unit & e2e & lint & build passing locally for me (e2e and unit both failed after I first pushed, so really thankfully our JS code is reasonably well covered as I had no recollection of why the test would fail :pray: )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] Gh 2767 fix js warnings [jena]
Posted by "kinow (via GitHub)" <gi...@apache.org>.
kinow commented on code in PR #2768:
URL: https://github.com/apache/jena/pull/2768#discussion_r1797717945
##########
jena-fuseki2/jena-fuseki-ui/package.json:
##########
@@ -40,6 +42,7 @@
"@cypress/code-coverage": "^3.12.4",
"@cypress/vue": "^6.0.0",
"@eslint/js": "^9.2.0",
+ "@types/codemirror": "^5.60.15",
Review Comment:
CodeMirror is used as we import & call some of its functions in our code (for query editor). We also import the `@types` lib for codemirror, but that's development dev only.
qs is the query string parser.
##########
jena-fuseki2/jena-fuseki-ui/src/App.vue:
##########
@@ -29,11 +29,13 @@
<script>
import Menu from '@/components/Menu.vue'
+import Toast from '@/components/Toast.vue'
export default {
name: 'App',
components: {
- Menu
+ Menu,
+ Toast
Review Comment:
We used it in the template, but did not import it. Vue/Vite were happy with that, but the IDE warns you when you don't do it (depending on your project setup, that may raise errors)
##########
jena-fuseki2/jena-fuseki-ui/src/utils/query.js:
##########
@@ -49,6 +49,6 @@ export function createShareableLink (query, path) {
path +
'?' +
// Same as YASGUI does, good idea to avoid security problems...
- queryString.stringify({ query })
+ qs.stringify({ query })
Review Comment:
Dropped `query-string`, using `qs` now (both were used), see issue main comment.
##########
jena-fuseki2/jena-fuseki-ui/src/mixins/current-dataset-navigation-guards.js:
##########
@@ -31,7 +31,7 @@ export default {
},
async beforeRouteUpdate (from, to, next) {
// eslint-disable-next-line no-unused-vars
- next(async vm => {
+ next(async _ => {
Review Comment:
So that it's ignored.
##########
jena-fuseki2/jena-fuseki-ui/src/services/fuseki.service.js:
##########
@@ -142,11 +142,11 @@ class FusekiService {
})
} catch (error) {
if (error.response) {
- if (error.response.status !== 200) {
- if (error.response.status === 409) {
+ if (error.response.statusCode !== 200) {
+ if (error.response.statusCode === 409) {
throw new Error(`failed to create dataset "${datasetName}", reason: there is another dataset with the same name`)
}
- throw new Error(`failed to create dataset "${datasetName}" with type ${datasetType}, reason: HTTP status: "${error.response.status}", message: ${error.response.statusText}`)
+ throw new Error(`failed to create dataset "${datasetName}" with type ${datasetType}, reason: HTTP status: "${error.response.statusCode}", message: ${error.response.statusText}`)
Review Comment:
Probably a bug :disappointed_relieved: `status` is a function, so it would never be `200`, `409`. The IDE warned me about the type mismatch, and looking at the code I realized we should have used `statusCode`. Fixed, and tests updated too.
The code would still fail to create the dataset, but I believe it was just not displaying a useful message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org