decentralized-identity / veramo

A JavaScript Framework for Verifiable Data
https://veramo.io
Apache License 2.0
414 stars 130 forks source link

Remove assert import for the plugin schema in favour of bundlers #1315

Closed cre8 closed 5 months ago

cre8 commented 5 months ago

What issue is this PR fixing

Fixes #1254

What is being changed

As described in the issue I replaced the assert { type: 'json' } with a classic typescript import. This allows other bundlers like webpack to deal with this without throwing an error since the assert command is not fully supported by typescript yet

I also stored the plugin.json file as non formatted, saving around 50% of the size.

There are still some assert imports in the ld-default-context package, this can be handled in another issue since it has nothing to do with this task of generating the schema of a plugin.

The plugin.schema.ts is generated in parallel to the plugin.schema.json so it does not break other project that are depending on them.

Quality

Check all that apply:

codecov-commenter commented 5 months ago

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (165de35) 85.50% compared to head (a4d41f5) 68.18%.

Files Patch % Lines
packages/core-types/src/plugin.schema.ts 0.00% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## next #1315 +/- ## =========================================== - Coverage 85.50% 68.18% -17.33% =========================================== Files 170 176 +6 Lines 18946 26993 +8047 Branches 2115 2116 +1 =========================================== + Hits 16200 18405 +2205 - Misses 2746 8588 +5842 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

cre8 commented 5 months ago

Btw it's not only a webpack problem, but also for esbuild and babel. For babel there is a plugin that can deal with plugin-syntax-import-assertions, but the other bundlers are lacking behind.

cre8 commented 5 months ago

So should we remove the json files from this PR and merge it when it fits for the major version?

mirceanis commented 5 months ago

So should we remove the json files from this PR and merge it when it fits for the major version?

Yes, please remove them.

We don't have an ability to mark them as deprecated, but if anyone depends on them they'll see a broken build after upgrading to the new major version and they'll be able to roll back their upgrade.

cre8 commented 5 months ago

Done

cre8 commented 5 months ago

@mirceanis I am not quite sure if import schema from '@veramo/core-types/build/plugin.schema' is the correct way to go. It seems better to export the plugin.schema.ts from the index.ts file so the import can be shortened. But for this I think we need also export the schema as a const and not a default.

mirceanis commented 5 months ago

I agree. Please post a PR if you're already testing this out

mirceanis commented 5 months ago

I agree. Please post a PR if you're already testing this out

@cre8 nevermind, I've managed to create a PR (#1327) as it was blocking another task I'm working on. Please take a look.