Skip to content

GH-2456: Change the project type to module, and update files to match the import type (import, from ES6)#2458

Merged
kinow merged 1 commit intomainfrom
use-type-module
May 7, 2024
Merged

GH-2456: Change the project type to module, and update files to match the import type (import, from ES6)#2458
kinow merged 1 commit intomainfrom
use-type-module

Conversation

@kinow
Copy link
Member

@kinow kinow commented May 6, 2024

GitHub issue resolved #2456
GitHub issue resolved #2056

Pull request Description:

Changes the project type to module, defining it as an ES6 project. That forces us to use only import everywhere, including Vite, ESLint, and Cypress configuration files.

This also fixes #2056 as it removes the CommonJS imports, using only ES6 import.


  • Tests are included.
  • Documentation change and updates are provided for the Apache Jena website
  • Commits have been squashed to remove intermediate development commit messages.
  • 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.


See the Apache Jena "Contributing" guide.

@kinow kinow force-pushed the use-type-module branch from ae88f51 to f731711 Compare May 6, 2024 20:33
@kinow kinow self-assigned this May 6, 2024
@kinow
Copy link
Member Author

kinow commented May 6, 2024

Doing it quickly today while it's fresh, as I fixed this on another project a few weeks/months ago. I remember it was annoying finding the right Cypress config, but luckily Jena is using a very similar configuration (no surprise 🤓 )

tupilabs/vue-lumino#130

@kinow kinow force-pushed the use-type-module branch 3 times, most recently from bc72428 to 9777888 Compare May 6, 2024 21:56
@kinow kinow marked this pull request as ready for review May 6, 2024 21:58
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This replaces .eslintrc.js. Can't find the link in their docs, but somewhere in the eslint 8.x to 9.x they defined the eslint.config.js was the preferred form (makes sense, I think, vite, cypress, etc., follow this pattern too).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I did the fix on the other project pretty quickly, except for this file. Finding the right config for Cypress was really frustrating. Hence, an important point to be reviewed, by generating the coverage for e2e tests 👍

"test:unit": "vitest run --environment jsdom",
"test:e2e": "cross-env FUSEKI_PORT=\"${FUSEKI_PORT:=3030}\" PORT=\"${PORT:=8080}\" concurrently --names 'SERVER,CLIENT,TESTS' --prefix-colors 'yellow,blue,green' --success 'first' --kill-others 'yarn run serve:fuseki' 'yarn wait-on http://localhost:${FUSEKI_PORT}/$/ping && yarn run dev' 'yarn wait-on http-get://localhost:${PORT}/index.html && cypress run $@'",
"lint": "eslint --ext .js,.vue --ignore-path .gitignore --fix src",
"lint": "eslint --fix src",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They dropped the --ext, and now the rest can all be configured in the config.js file.

"sinon": "^17.0.0",
"vite": "^5.0.12",
"vite-plugin-istanbul": "6.0.0",
"vite-plugin-istanbul": "6.0.2",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Le fix for the dependabot upgrade bug.

* limitations under the License.
*/

import { createApp, h } from 'vue'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New version of eslint found several issues (I think we were ignoring some too), so remaining changes is mainly fixing that.

this.loadingGraph = true
this.selectedGraph = graphName
try {
const dataEndpoint = this.services['gsp-rw']['srv.endpoints'].find(endpoint => endpoint !== '') || ''
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not used.

server.get('/tests/reset', (req, res) => {
// Just delete the datasets to clean up for other tests to have a
// brand new environment.
// brand-new environment.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, IDE was a bit picky, so including this change just to satisfy it so that it doesn't show an annoying warning.

cy.request({
url: '/tests/reset',
retryOnStatusCodeFailure: true
})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be my environment, but for some unknown reason the fake Fuseki used in the e2e tests started failing intermittently.

Sometimes everything worked fine, sometimes the second or third test failed.

Tried debugging it, changing settings, but couldn't understand what's happening. Looked at the issues of Json-server but didn't find anything helpful (the test fails with HTTP error 500 on the Json-server reply).

Cypress contains a setting to retry (4 times by default), which fixed the test on my environemnt. Not sure what could have caused it.

@kinow kinow changed the title Change the project type to module, and update files to match the import type (import, from ES6) GH-2456: Change the project type to module, and update files to match the import type (import, from ES6) May 6, 2024
@kinow
Copy link
Member Author

kinow commented May 6, 2024

Ready for review 👍

@kinow kinow requested a review from afs May 6, 2024 22:03
"sinon": "^17.0.0",
"vite": "^5.0.12",
"vite-plugin-istanbul": "6.0.0",
"vite-plugin-istanbul": "6.0.2",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe put back the ^ which was removed in the #2457 quick-fix.

Suggested change
"vite-plugin-istanbul": "6.0.2",
"vite-plugin-istanbul": "^6.0.2",

At this point in time it resolves to 6.0.2 and the yarn.lock file only changes to reflect adding the ^.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed that, sorry. It's back now 👍

@kinow kinow force-pushed the use-type-module branch from 9777888 to 51bd345 Compare May 7, 2024 20:11
@kinow kinow force-pushed the use-type-module branch from 51bd345 to dd5fb33 Compare May 7, 2024 20:48
version "6.0.0"
resolved "https://registry.yarnpkg.com/vite-plugin-istanbul/-/vite-plugin-istanbul-6.0.0.tgz#630c1404c09ae242f84b819f001abe4db3f8bf2e"
integrity sha512-Vwh2XdesjcLwaPbHSOiWHh+0s7CNovQTPEjUCTkqmJUe0FN2TKsOp0qpoaklOuwrKlL9elhD5fPFxi5lmG62zA==
vite-plugin-istanbul@^6.0.2:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^6.0.2 ☝️

"sinon": "^17.0.0",
"vite": "^5.0.12",
"vite-plugin-istanbul": "6.0.0",
"vite-plugin-istanbul": "^6.0.2",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^6.0.2 👍 ☝️ @afs

@kinow
Copy link
Member Author

kinow commented May 7, 2024

Executed lint and unit/e2e tests, all passing, merging now. Thanks!

@kinow kinow merged commit c93343b into main May 7, 2024
@kinow kinow deleted the use-type-module branch May 7, 2024 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vite-plugin-istanbul 6.0.1+ not compatible with Fuseki UI build. [jena5] Fix deprecation warnings in upcoming Vite release

2 participants