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

Feature: migrate to angular 6 #295

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

hamzahamidi
Copy link

@hamzahamidi hamzahamidi commented Jun 28, 2018

PR Type

What changes does this PR include (check all that apply)?
[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build process changes
[ ] Documentation changes
[ ] Other... please describe:

Related issue / current behavior

The library is currently incompatible with Angular v6 mainly due to some changes in RXJS v6, material Angular v6 & Angular v6

New behavior

  • Update the library to Angular v6.
  • Migrate to RXJS V6
  • Fix issue 281 ( duplicate AJV imports in build)

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Any other relevant information

@hamzahamidi
Copy link
Author

hamzahamidi commented Jun 29, 2018

For people who can't wait for this update to get merged here's a quick fix:

$ npm install git+https://github.com/hamzahamidi/angular2-json-schema-form.git#build-angular-6 --save

You may need to install @angular/flex manually: $ npm i @angular/flex-layout
Once this branch get merged, I'll delete the branch so it will not work anymore, in respect to the repo's owner @dschnelldavis

@maher1991
Copy link

After downloading the code of the angular6 migration, what are the steps to build the lib locally and use it in my own projects ?

@hamzahamidi
Copy link
Author

hamzahamidi commented Jul 2, 2018

@maher1991 If you want to add the library to your project, just run the commands above in the root of your project & then Then import JsonSchemaFormModule in your main application module, like this:

import { BrowserModule } from '@angular/platform-browser';
import { NgModule } from '@angular/core';

import {
  JsonSchemaFormModule, MaterialDesignFrameworkModule
} from 'angular2-json-schema-form';

import { AppComponent } from './app.component';

@NgModule({
  declarations: [ AppComponent ],
  imports: [
    BrowserModule, MaterialDesignFrameworkModule,
    JsonSchemaFormModule.forRoot(MaterialDesignFrameworkModule)
  ],
  providers: [],
  bootstrap: [ AppComponent ]
})
export class AppModule { }

@maher1991
Copy link

It works fine. Thanks

@Goopil
Copy link

Goopil commented Jul 3, 2018

Hello there, do you plan to merge this ?

@catull
Copy link

catull commented Jul 3, 2018

@Goopil
A couple of observations.

maher1991 is not a main contributor to this project, thus he cannot merge PRs.

This project has not been actively contributed to since February 4th, 2018.

Looking at contributors you see the four main committers have not made a change for quite some time.

My advise is to follow shamoons' fork, whose author has stated he wants to dedicate his spare time to improving the project.

@Goopil
Copy link

Goopil commented Jul 3, 2018

Thanks for the advise. I will do just that.

@hamzahamidi
Copy link
Author

@Goopil I'm not the repo's owner.
@catull the other fork is still in v5

@Goopil
Copy link

Goopil commented Jul 3, 2018

@hamzahamidi Yes i've seen it. What do you plan to do with your fork ?

@hamzahamidi
Copy link
Author

@Goopil I'm hoping it get merged in the main repo. We use this library in big projects, so we really need this update. In the mean time we're using my fork.

@Goopil
Copy link

Goopil commented Jul 3, 2018

@hamzahamidi we are in the same situtation. i hope it will be merged soon

@zuozhiw
Copy link

zuozhiw commented Jul 3, 2018

@hamzahamidi I really appreciate your work on providing us an Angular 6 build. Thanks a lot.

When I use this build, I found using schema involving enum will lead to errors, the stack trace looks like this:

PropertyEditorComponent.html:6 ERROR RangeError: Maximum call stack size exceeded
    at isNumber (angular2-json-schema-form.es5.js:67)
    at isEqual (angular2-json-schema-form.es5.js:6562)
    at isEqual (angular2-json-schema-form.es5.js:6566)
    ......

After digging around for a while, I found the cause of the problem is this function in the JS file after build (line 5650 in angular2-json-schema-form.es5.js)

var isEqual = function (enumValue, inputValue) {
    return enumValue === inputValue ||
        (isNumber(enumValue) && +inputValue === +enumValue) ||
        (isBoolean(enumValue, 'strict') &&
            toJavaScriptType(inputValue, 'boolean') === enumValue) ||
        (enumValue === null && !hasValue(inputValue)) ||
        isEqual(enumValue, inputValue);
};

The problem is that the last function call isEqual recursively calls itself. However, when we look at the original TS file:

const isEqual = (enumValue, inputValue) =>

The last line of function isEqual is actually:_.isEqual(enumValue, inputValue);, it's not calling it self, rather it's calling lodash's isEqual.

The problem wasn't in the current master build. This is what looks like in the current build:

var isEqual$$1 = function (enumValue, inputValue) {
    return enumValue === inputValue ||
        (isNumber(enumValue) && +inputValue === +enumValue) ||
        (isBoolean(enumValue, 'strict') &&
            toJavaScriptType(inputValue, 'boolean') === enumValue) ||
        (enumValue === null && !hasValue(inputValue)) ||
        isEqual(enumValue, inputValue);
};

We can see that the function isEqual is actually replaced with name isEqual$$1.

It seems like when we update to Angular 6, the library build process is messed up. I'm worried about this issue could potentially cause more bugs. I tried to find what configuration caused the exact problem, but I'm currently clueless. I'll continue finding the solution, @hamzahamidi I would really appreciate it if you or anyone else could help in solving this problem.

@hamzahamidi
Copy link
Author

@zuozhiw Thank you for your feedback. Can you provide me please with your json schema, browser, node & npm version to reproduce the error? Thank you again for your feedback.

@zuozhiw
Copy link

zuozhiw commented Jul 3, 2018

@hamzahamidi Thanks for the reply. Here's how to reproduce:

Json Schema:

{
  "properties": {
    "options": {
      "type": "string",
      "enum": [ "a", "b", "c"]
    }
  },
  "type": "object"
}

On the generated form, click option "b" or "c" in the drop down menu, then you can see the error in console.

Versions:
Chrome
Node 8.11.3
NPM 5.6.0
Angular CLI 6.0.8,
Angular 6.0.7,
Angular material 6.3.2,
Angular flex-layout 6.0.0-beta.16
Typescript 2.7.2

@SamuelMarks
Copy link
Contributor

@zuozhiw In the meantime, try rewriting it in ES6 syntax:

const isEqual = (enumValue, inputValue) =>  enumValue === inputValue ||
        (isNumber(enumValue) && +inputValue === +enumValue) ||
        (isBoolean(enumValue, 'strict') &&
            toJavaScriptType(inputValue, 'boolean') === enumValue) ||
        (enumValue === null && !hasValue(inputValue)) ||
        isEqual(enumValue, inputValue);

Though honestly, that last call to isEqual is suspect. It's never expanding the enum to check inside. You might want to throw in a .every

@hamzahamidi
Copy link
Author

hamzahamidi commented Jul 4, 2018

This Repo also is compatible with Angular v6 & follows Angular v6 best practices for generating libraries.
$ npm run buildlib to generate the bundles I'll make an npm repo asap

Edit:

I transferred the Repo in my personal GitHub

@zuozhiw
Copy link

zuozhiw commented Jul 5, 2018

@hamzahamidi I've tested your new repo locally and it now works for the bug I encountered 👍 Thanks a lot. We are also upgrading to Angular 6 and you saved the build!

A couple of things that I suggest:

  1. In the original repo, it has all the angular stuff and rxjs as peer dependency, has ajv and lodash as dependency. In your new repo, it currently lists all the angular stuff as dependency, and I can't find ajv and lodash in package.json. When I try to link the repo, it prompts me to install ajv and lodash as peer dependencies. I'm not familiar with the new Angular build process, but I'm wondering if the new package.json should mimic what's in the old package.json?

  2. Although the name of the github repo is Angular6-json-schema-form, the package name is json-schema-form, I believe this name is already taken in npm. You might need to rename it.

@jakubjosef
Copy link

@hamzahamidi Thank you very much, your version is working but unfortunately layouting not works for me. Generated form just ignore the layout json... :( And this is most important feature. Can you find some time to test it on your own?

@jakubjosef
Copy link

JFYI: @hamzahamidi doing great job with this version
https://github.com/hamzahamidi/Angular6-json-schema-form/

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.

7 participants