Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Jan 24, 2024

Conversation

cre8
Copy link
Contributor

@cre8 cre8 commented Jan 17, 2024

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:

  • I want these changes to be integrated
  • I successfully ran pnpm i, pnpm build, pnpm test, pnpm test:browser locally.
  • I allow my PR to be updated by the reviewers (to speed up the review process).
  • I added unit tests.
  • I added integration tests.
  • I did not add automated tests because no new functionality was added, and I am aware that a PR without tests will likely get rejected.

cre8 added 2 commits January 17, 2024 18:25
Fixes decentralized-identity#1254

Signed-off-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
Signed-off-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
@codecov-commenter
Copy link

codecov-commenter commented Jan 18, 2024

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 ⚠️
Additional details and impacted files
@@             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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cre8 cre8 changed the title Remove assert import for the plugin schema in favour of webpack Remove assert import for the plugin schema in favour of bundlers Jan 18, 2024
@cre8
Copy link
Contributor Author

cre8 commented Jan 18, 2024

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.

Copy link
Member

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

This looks good to me.
Since these are generated files, they will get updated as the plugin schemas change (even when updating inline docs) so it will lead to ugly diffs and conflicts.
I think we shouldn't care about having the outputs minified (or in our case not formatted) since that's a task for bundlers.

Also, this will get merged into next which is scheduled for a release soon including other breaking changes that will trigger a major version bump so I would opt for removing the JSON files completely

@cre8
Copy link
Contributor Author

cre8 commented Jan 19, 2024

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

@mirceanis
Copy link
Member

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.

Fixes decentralized-identity#1254

Signed-off-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
@cre8
Copy link
Contributor Author

cre8 commented Jan 19, 2024

Done

Signed-off-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
Copy link
Member

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

This looks good.
Thank you for posting these changes.

@mirceanis mirceanis merged commit 65c2d4b into decentralized-identity:next Jan 24, 2024
9 checks passed
@cre8
Copy link
Contributor Author

cre8 commented Jan 25, 2024

@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
Copy link
Member

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

@mirceanis
Copy link
Member

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.

mirceanis added a commit that referenced this pull request Jan 25, 2024
* remove references to the JSON build output for plugin schemas
* export the schemas from each package

relates to #1318 / #1317 / #1315
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove assert import in favour of webpack
3 participants