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

fix : tv4 as dependency #5

Merged
merged 3 commits into from
May 15, 2017

Conversation

jsomsanith-tlnd
Copy link

Description

We cannot use json-schema-form-core as an npm lib, du to webpack config that declares tv4 as externals.

Installation :

yarn add json-schema-form-core

Use :

import { merge } from 'json-schema-form-core';

Result:

ReferenceError: tv4 is not defined
    at Object.eval (webpack:///./~/json-schema-form-core/dist/json-schema-form-core.js?:2623:18)
    at __webpack_require__ (webpack:///./~/json-schema-form-core/dist/json-schema-form-core.js?:29:30)
    at Object.module.exports.ObjectPath.parse.i (webpack:///./~/json-schema-form-core/dist/json-schema-form-core.js?:2452:62)
    at __webpack_require__ (webpack:///./~/json-schema-form-core/dist/json-schema-form-core.js?:29:30)
    at Object.module.exports._typeof (webpack:///./~/json-schema-form-core/dist/json-schema-form-core.js?:491:72)
    at __webpack_require__ (webpack:///./~/json-schema-form-core/dist/json-schema-form-core.js?:29:30)
    at Object.eval (webpack:///./~/json-schema-form-core/dist/json-schema-form-core.js?:2629:18)
    at __webpack_require__ (webpack:///./~/json-schema-form-core/dist/json-schema-form-core.js?:29:30)
    at module.exports.Object.defineProperty.value (webpack:///./~/json-schema-form-core/dist/json-schema-form-core.js?:75:18)
    at Object.eval (webpack:///./~/json-schema-form-core/dist/json-schema-form-core.js?:78:10)

Motivation :

  • to fix this, it requires extra webpack configuration in the user project to declare it as externals.
  • this should work out of the box, and not let the user fix that

Fixes Related issues

Checklist

  • I have read and understand the CONTRIBUTIONS.md file --> where is it ?
  • I have searched for and linked related issues
  • I have created test cases to ensure quick resolution of the PR is easier
  • I am NOT targeting main branch
  • I did NOT include the dist folder in my PR

@json-schema-form/angular-schema-form-lead

@Anthropic
Copy link
Member

Thanks @jsomsanith the webpack file change will cause the entire tv4 lib to be imported into the library on build. Which negates the need for the other change, however I prefer the package file change only as I don't want to include tv4 as I hope to make it optional in a future update. If you add it back happy to accept the package change, that was my mistake :)

@jsomsanith-tlnd
Copy link
Author

Hi @Anthropic, I'm not sure to understand everything you said.
Which negates the need for the other change --> what do you call the other change ?
I prefer the package file change only --> what do you call the package file change ? Do you mean to install tv4 and to add it as external in webpack config ?

Do you agree that the current situation is problematic ? (What I wrote in motivation part). If so, let's find a solution, because a lib should not be that hard to use.

@jmfrancois
Copy link

jmfrancois commented May 7, 2017

Yes but at the moment you ask to your users to configure webpack the same way you are doing it (with extra bundle).

With the following change the user will not have to care about what is used to validate the JSON schema.
So with this current change:
I can use json-schema-form-core without care of what is the validation library inside.
So if you do a change one day from tv4 to sth else, this is not a problem for your user.

If you let the current situation, all the users are doing specific webpack config to add tv4.
Then you change from tv4 to sth else, all the other users will have to change their webpack config.

This is a dependency so I should not take care of it in my build.

@jsomsanith-tlnd
Copy link
Author

@jmfrancois is right, this means you force everyone to update their webpack config when you change the dependency. That makes the future upgrades possibly harder or more annoying.
Obviously, for now tv4 is a dependency, without it this lib doesn't work, it is ok to have it in the bundle.

@Anthropic
Copy link
Member

@jsomsanith yes I agree, tv4 is problematic, I am hoping before I even get to a v1.0.0 release to make it optional, tv4 isn't supported at all any more, it is a real problem as many user issues are raised that are directly caused by it and I can't make changes to. The sooner I can remove it the better, you'll feel the same way very quickly :) I'm just trying to find the best way to do that.

I do take the point that I could include it until that time. However, that means making a breaking change against all the users who include it separately now and then another breaking change when the alternative is ready, I had hoped to only annoy everyone once. How are you including the lib, are you not using webpack?

Anyway what I meant about negating the other change was that if you include tv4 in the build then it should remain as a dev dependency only as it will not need to be included as a lib dependency, so I think you only want the change in the webpack file?

@jsomsanith-tlnd
Copy link
Author

We are not using the lib yet, but we plan too. We encountered the problem at the beginning of a POC, that's why I made the PR.
Ok I did not understand that it was in your immediate goal. But please, keep what we said in mind when you'll make the change. I mean that the users should not have to configure anything in their webpack to make the lib work.

Thank you for your time.

@Anthropic
Copy link
Member

@jsomsanith @jmfrancois I think we can discuss such things further here: https://gitter.im/json-schema-form/core

Be nice to communicate more fluidly about what you feel the API may need or things that could make it more useful before I actually start working on it in more detail. I originally expected it to be a pre-packed lib that can be re-packed by any lib wanting to embed it. But I have also recently been thinking about webpack tree shaking and can see the benefit in making it includable as a module that can be stripped of unused features.

But I am happy to accept this PR if you undo the package.json file change so that TV4 gets included for now if you like?

@Anthropic
Copy link
Member

Anthropic commented May 11, 2017

@jsomsanith I've added the contributing file, thanks for letting me know it was missing.

Update to package.json to change tv4 as it is already included within the library after the webpack change.
@Anthropic Anthropic merged commit 0de8649 into json-schema-form:development May 15, 2017
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.

None yet

4 participants