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

always reuse AJV instance from store #1624

Merged
merged 3 commits into from
Aug 21, 2020

Conversation

DBosley
Copy link
Contributor

@DBosley DBosley commented Aug 19, 2020

This PR changes packages/core/src/util/runtime.ts so it no longer creates an instance of AJV on file load. It will only run createAjv() if an instance of AJV is not supplied to any of the exported utilities. This moves the project closer to full abstraction of AJV to allow it as a peer dependency.

This change will allow users to inject their own instances of AJV and that instance will be used throughout the library. It allows a user to create mocks or delegates for core AJV functions which can be leveraged to void unsafe-eval errors in projects with CSP enabled.

#1498 #1557

@DBosley DBosley force-pushed the DBosley/fix-ajv-utility-usage branch from c8968df to 57af381 Compare August 19, 2020 18:41
Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!

  • We can assume that Ajv will always be in the state as we require that the init action was executed before rendering. Therefore we can just always extract Ajv from the state without creating a new instance when it doesn't exist (as this has already happened if needed in the init action).
  • I would also prefer not giving Ajv as a prop to all renderers as no renderer besides MaterialCategorizationLayout and MaterialCategorizationStepperLayout care about it. Instead we could add a withAjv HOC which can be used by any renderer additionally to their withJsonFormsXXXProps HOC, e.g. withAjv(withJsonFormsLayoutProps(MaterialCategorizationLayoutRenderer)).

packages/angular-material/src/other/label.renderer.ts Outdated Show resolved Hide resolved
packages/core/src/reducers/core.ts Outdated Show resolved Hide resolved
packages/core/src/util/cell.ts Outdated Show resolved Hide resolved
packages/core/src/reducers/index.ts Outdated Show resolved Hide resolved
packages/core/src/util/index.ts Outdated Show resolved Hide resolved
packages/core/src/util/renderer.ts Outdated Show resolved Hide resolved
packages/core/src/util/renderer.ts Outdated Show resolved Hide resolved
packages/core/src/util/renderer.ts Outdated Show resolved Hide resolved
@DBosley
Copy link
Contributor Author

DBosley commented Aug 21, 2020

Let me know if there's anything I can do to expedite getting this merged in and published to NPM. Once it is, I can begin figuring out ways to avoid usafe-evals with packed AJV modules.

Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

I removed the remaining default Ajvs and fixed the then failing testcases. Looks good now to me!

packages/core/src/util/runtime.ts Outdated Show resolved Hide resolved
@sdirix sdirix merged commit 0595f77 into eclipsesource:master Aug 21, 2020
@DBosley
Copy link
Contributor Author

DBosley commented Aug 21, 2020

Will this be released to NPM soon, or will this have to wait for a future publish? I made sure the utility functions all had defaults so there would be no breaking changes. I'm hoping it will be possible to publish it as a patch and a not a full major or minor version increment. As I mentioned before, this PR should unblock my work on my own project and let me figure out the details for implementing the packed AJV injector/wrapper.

I don't mean to rush you or your team, I'm just trying to get an estimate so I can relay the info back to my team.

@DBosley DBosley deleted the DBosley/fix-ajv-utility-usage branch August 21, 2020 15:38
@sdirix
Copy link
Member

sdirix commented Aug 21, 2020

I made sure the utility functions all had defaults so there would be no breaking changes

There should not be any breaking change here. At least not when only using JsonForms. When reusing code in custom renderers this will be a breaking change but a very easy one to fix.

Will this be released to NPM soon, or will this have to wait for a future publish?

I'm in the progress of releasing the first beta of 2.4.1 which you can then consume. The stable release will probably be in mid to end of September.

@sdirix
Copy link
Member

sdirix commented Aug 21, 2020

Release is done. You can consume it via @jsonforms/core@next or @jsonforms/core@2.4.1-beta.0 (same for other packages). Thanks again for the contribution!

@DBosley
Copy link
Contributor Author

DBosley commented Aug 21, 2020

Thank you!

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.

2 participants