gramps-project / gramps-web

Frontend for Gramps Web
https://www.grampsweb.org
GNU Affero General Public License v3.0
336 stars 48 forks source link

Import assertion syntax deprecated #415

Closed DavidMStraub closed 4 days ago

DavidMStraub commented 3 months ago

We use the import assertion syntax for importing language JSONs. They don't work in Firefox, which is why we have to use Chrome for development. I now started getting

'assert' is deprecated in import statements and support will be removed in V8 v12.6 and Chrome 126; use 'with' instead

in Chrome.

DavidMStraub commented 3 months ago

Release date for Chrome 126 is May 13, 2024.

DavidMStraub commented 3 months ago

@khashashin since you contributed the original version with import assertions, I'm wondering whether you'd like to have a look at this :slightly_smiling_face: Question 1 is how we fix it in Chrome (probably a simply syntax change), Question 2 how we have to change the rollup build.

khashashin commented 3 months ago

Thank you, @DavidMStraub, for bringing this up. I've submitted an intermediate PR

https://github.com/gramps-project/gramps-web/pull/416

that transitions from the deprecated assertion syntax to the newly accepted attribution syntax using with. Unfortunately, we're currently unable to proceed with merging this PR because our build process, specifically with Rollup, does not yet support the with syntax for import attributes.

I've looked into the availability of a Rollup plugin that could handle the with syntax but haven't found any existing solutions. It raises the question of whether we should develop a custom solution or wait for one to become available. @swiing, do you have any plans or insights regarding the development of such a plugin? Your input would be invaluable here.

I must note that the changes in the PR have not been extensively tested, primarily due to the build process limitations. To circumvent pre-commit hook restrictions that prevented pushing these changes, I used the --no-verify flag during the commit. This workaround isn't ideal, but it was necessary under the circumstances.

For the time being, my recommendation is to closely monitor the situation and adapt our approach as the ecosystem evolves to fully support the with syntax.

DavidMStraub commented 3 months ago

Thanks a lot for looking into this so quickly!

swiing commented 3 months ago

@khashashin try to upgrade to rollup v4. If it doesn’t work for you, please open an issue in the rollup-plugin-import-assertions

khashashin commented 3 months ago

Hey @DavidMStraub, @swiing suggested upgrading to Rollup v4 to support the with syntax for our imports. What do you think? Any big concerns or an idea of how long it might take us to switch? Just trying to figure out if it’s a smooth update or if we’re looking at a bigger project. Thanks!

DavidMStraub commented 2 months ago

Hi, I looked into it quickly and I'm not sure - right now we're using @open-wc/building-rollup and unforunately it looks like this still doesn't support Rollup v4 (only v3).

khashashin commented 2 months ago

@DavidMStraub ok... I noticed that they are also tracking the issue of import assertion https://github.com/open-wc/open-wc/issues/2700

DavidMStraub commented 1 week ago

My development environment stopped working as my Chromium was upgraded to v126, but the problems we discussed are still not resolved. Not sure what's the best way forward.

khashashin commented 1 week ago

Eventually, the easiest way to solve this problem would be to convert all json files into javascript files. But sticking with outdated build packages is not an optimal solution either. I would suggest that we try to use js instead of json and start the migration to rollup v4 in parallel.

khashashin commented 1 week ago

I've updated the PR

DavidMStraub commented 1 week ago

But this will break the connection to Weblate. I don't think Weblate supports .js for translation files.

Another workaround would be to fetch the JSONs, but that would lead to additional HTTP requests and we would have to add additional failure handling in case the fetch fails, etc.

khashashin commented 1 week ago

I see... those json files are used by weblate? I thought they were just static files to do the translation for the part of the app that is not translated with weblate.

Yes, retrieving the json files (maybe even directly from github) would be a handy workaround that would take less time than migrating to rollup v4.

DavidMStraub commented 1 week ago

I looked into this today for a while but it's quite difficult. We would first have to move all JSONs from src/lang, which requires careful changes to the Weblate settings to not break everything (https://docs.weblate.org/en/latest/faq.html#how-to-move-files-in-the-repository-without-losing-history-in-weblate). :face_with_diagonal_mouth:

khashashin commented 1 week ago

Well, I have no experience with Weblate and can't help here. It seems to me that the integration of all the app components and the automation of translations is quite challenging.

DavidMStraub commented 6 days ago

Hi @Nick-Hall, I think we have to move the translation files from src/lang/*.json to lang/*.json to use fetch instead of import assertions. Can you please execute the following steps for the Gramps Web component on Weblate?

  • Lock the affected component in Weblate. ...
  • Disable receiving webhooks the Project configuration; this prevents Weblate from immediately seeing changes in the repository.

Thanks!

Nick-Hall commented 5 days ago

@DavidMStraub Done.

DavidMStraub commented 5 days ago

Thanks! But please don't push to main next time. You broke the development version & images which some people use for deployments. I wanted you to do those two steps on Weblate and then wait until I have implemented the necessary changes in a PR.

DavidMStraub commented 4 days ago

Done, we are no longer using import assertions.

khashashin commented 4 days ago

So you have included the json files in the bundle, if the size of the bundle is not a concern I think that is a pragmatic solution. Would you like me to update the PR, which should now simply remove the package?

Nick-Hall commented 4 days ago

Thanks! But please don't push to main next time.

@DavidMStraub Sorry. I interpreted the ellipsis in the list as additional steps that you wanted me to complete.

DavidMStraub commented 3 days ago

Would you like me to update the PR, which should now simply remove the package?

Oh right, we still have to remove the unneeded dependencies.