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

Using TypeScript in Zeppelin frontend webapp #321

Closed
wants to merge 29 commits into from

Conversation

bzz
Copy link

@bzz bzz commented Feb 6, 2015

As discussed in this @dev thread this is the intial step on using TypeScript

TODOs:

  • add initial frontend build integration
  • replace test.ts with something more useful
  • add yoman integration

@swkimme
Copy link
Contributor

swkimme commented Feb 8, 2015

@bzz
I've move all js files into ts files. It works like a charm with little efforts!
I needed to modify the way we manage js files, would you review them??

@anthonycorbacho
Copy link
Contributor

Why are you commenting the process of minify ,uglify and concat the js?

@swkimme
Copy link
Contributor

swkimme commented Feb 8, 2015

it was my mistake, reverted by additional commit.

On Sun Feb 08 2015 at 2:24:08 PM Anthony Corbacho notifications@github.com
wrote:

Why are you commenting the process of minify ,uglify and concat the js?


Reply to this email directly or view it on GitHub
#321 (comment).

@swkimme
Copy link
Contributor

swkimme commented Feb 9, 2015

I've setup initial structures for each controllers (nav, notebook, paragraph)
I just described current status as interfaces and didn't changed code structure except minor refactors,
It should just work well.

@corneadoug
Copy link
Contributor

Would be pretty cool if that kind of base interfaces were generated when we create controllers using yeoman.

Got a few questions after checking the code:

  • Why are some classes interface? (for exemple: export interface Notebook), wouldnt class be enough?
  • Why are some interface starting with 'I' and some not (export interface Notebook, interface INotebookCtrlScope)

Not much more to say, it looks like a good job. However I would have prefered if the refactoring was done in a different commit, it gets confusing when we try to understand the changes in synthax of typescript :)

@swkimme
Copy link
Contributor

swkimme commented Feb 9, 2015

@corneadoug
Nice catch! Updated Notebook and NotebookConfig to class from interface.
I'm not very sure it's good to using interface or class for that kind of objects for now (Notebook, Paragraph, ..) I guess we need some try for now.

I'll try not modifying other codes when doing this kind of big code migrating job in the future, for code review readability. And also try yoman integration soon!

@swkimme
Copy link
Contributor

swkimme commented Feb 9, 2015

BTW, why the travis build fail on zeppelin-server? Do I need to merge master branch?

…ent/add-typescript

Conflicts:
	zeppelin-web/app/scripts/controllers/interpreter.js
@swkimme
Copy link
Contributor

swkimme commented Feb 9, 2015

manually merged #330

…ent/add-typescript

Conflicts:
	zeppelin-web/app/scripts/controllers/interpreter.js
	zeppelin-web/app/scripts/controllers/main.js
	zeppelin-web/app/scripts/controllers/nav.js
	zeppelin-web/app/scripts/controllers/notebook.js
	zeppelin-web/app/scripts/controllers/paragraph.js
@swkimme
Copy link
Contributor

swkimme commented Feb 10, 2015

Anyone can help fixing the travis build?
https://travis-ci.org/NFLabs/zeppelin/builds/50077915
It seems like failing on tests, but hard to determine where it fails
@bzz @Leemoonsoo

ADD:
Resolved, it was because the new way loading angular app doesn't work with ngmin. Reverted changes to fix it.

@bzz
Copy link
Author

bzz commented Feb 10, 2015

Cool, glad you solved it!
Usually that means the integration tests are failing zeppelin-server/src/test/scala/com/nflabs/zeppelin/*.scala with scalatest, without proper propagation to TravisCI.

@swkimme
Copy link
Contributor

swkimme commented Feb 10, 2015

I'd like to finalize my changes with this PR, please review this branch!
@Leemoonsoo @anthonycorbacho @bzz @corneadoug

@swkimme
Copy link
Contributor

swkimme commented Feb 10, 2015

Here's my work log

  • move js files into ts files
  • added typed interfaces, classes to describe major objects in controllers
  • defined events and constructors in event.ts
  • tried to use enum-like static members for strings (enum fails..)
  • divide huge paragraph controller into 3 parts, paragraph, paragraph_editor, paragraph_result
  • refactored some codes for shorter or safer expression.
  • removed many redundant or unused codes

@corneadoug
Copy link
Contributor

The refactoring looks pretty nice, the code dividing too.
Let's try to finalize the build/testing/documentation/tooling/code generation part and that will be good to go.

@swkimme
Copy link
Contributor

swkimme commented Feb 11, 2015

@corneadoug
Thanks! Now I'm working on docs and trying lint tools as @anthonycorbacho mentioned in #305.

For building, testing, code generation (yeoman), it's quite unfamiliar to me so it will be cool if experts help or assist me.

@swkimme
Copy link
Contributor

swkimme commented Feb 11, 2015

I've updated docs about generating codes via yeoman.
I was able to generate ts codes, need test if it works well with existing codes.

Also it generates ts codes under script/controller/ but current ts code location is script/ts/controller.
Any idea on this? It'll be good if I put ts codes in script/controller/ with js files?
@anthonycorbacho @corneadoug

…escript

Conflicts:
	spark/src/main/java/com/nflabs/zeppelin/spark/SparkSqlInterpreter.java
@Leemoonsoo
Copy link
Contributor

While PR brings a lot of change, every other PR that make update on zeppelin-web is going to conflict with this PR.

This branch is working well. I think it's better to merge and handle remaining issue separately. Before more PR conflicts with it.

@anthonycorbacho
Copy link
Contributor

no, I would like to handle everything before merging.
On Feb 13, 2015 7:53 PM, "Lee moon soo" notifications@github.com wrote:

While PR brings a lot of change, every other PR that make update on
zeppelin-web is going to conflict with this PR.

This branch is working well. I think it's better to merge and handle
remaining issue separately. Before more PR conflicts with it.


Reply to this email directly or view it on GitHub
#321 (comment).

@swkimme
Copy link
Contributor

swkimme commented Feb 13, 2015

@anthonycorbacho
Can you be more specific on what do we have to do more for this PR?
(For code generating thing, I'm not very sure we need code generation template at this stage.. as already we got good samples when we create new controller or other components)

@corneadoug
Copy link
Contributor

What is the point of going to typescript is its to break the way we the app
build, make the branch less stable, and make it harder for people to code.
If this is the way we want to handle adding new development tools then we
should just drop typescript, adding new development tool means it has to
the project, not the other way around.This is not a dev branch, this is the
master branch that everybody builds from since we don't curently have
releases. I already mentioned what was left some comments before. I can
make a bullet list in the PR if you want
On Feb 13, 2015 8:14 PM, "Kevin (Sangwoo) Kim" notifications@github.com
wrote:

@anthonycorbacho https://github.com/anthonycorbacho
Can you be more specific on what do we have to do more for this PR?
(For code generating thing, I'm not very sure we need code generation
template at this stage.. as already we got good samples when we create new
controller or other components)


Reply to this email directly or view it on GitHub
#321 (comment).

@swkimme
Copy link
Contributor

swkimme commented Feb 13, 2015

@corneadoug
I understood. I've updated not to change build structure to keep the way app build. Now it just build like before.

Would you comment more on toolings and testing? What kind of tools and tests are we using now?
And do you have some comment on code generation part? In my thought it's less important part for us in this stage.

  • build - building front (.ts to .js)
  • build - grunt serve to run web from source
  • build - generate the .ts from yeoman
  • documentation - update on Typescript
  • documentation - how to use class, interface, export
  • bonus - add StyleCop
  • bonus - test Karma

@corneadoug
Copy link
Contributor

I will help on this PR this week end to finish the merge to typescript.

We are using grunt and yoman.

On the test we are using jshint that can be replaced by tslint and I guess
typescript also have some kind of error check with the types.

Basically the scenario we want are:

  • Building front (transforming .ts to .js)
  • Being able to run grunt serve to run the web from source (so from .ts?),
    if we have to transform to .js, then maybe just skip minification and have
    a fast build on the code watch could work.
  • Have a way to generate the .ts from yoman (yoman on top of creating the
    file takes care of registering the components in the app.js)
  • Have a documentation on how to create new controllers, directive etc...
    and example on how we use the class, interface, export.

Bonus/Can be done in a different PR

  • Creating example or empty class and interface when we use yoman
  • Add a StyleCop type of plugin
  • See if karma is typescript friendly, and id testing is different with
    typescript (we curently dont have test)
    On Feb 13, 2015 10:12 PM, "Kevin (Sangwoo) Kim" notifications@github.com
    wrote:

@corneadoug https://github.com/corneadoug
I understood. I've updated not to change build structure to keep the way
app build. Now it just build like before.

Would you comment more on toolings and testing? What kind of tools and
tests are we using now?
And do you have some comment on code generation part? In my thought it's
less important part for us in this stage.

  • build
  • testing
  • documentation
  • tooling
  • code generation part


Reply to this email directly or view it on GitHub
#321 (comment).

@swkimme
Copy link
Contributor

swkimme commented Feb 13, 2015

@corneadoug
Great, thanks! Your help will be BIG.

I watched Angular2 (alpha) demo today,

http://youtu.be/uD6Okha_Yj0?t=8m51s

It implies Angular2 will adopt ATScript ( @bzz mentioned it on mailing list, it's Typescript superset), it will be good for us to prepare Typescript or at least, we can think we're going to right way!
(I think even it will be good for us to get ATScript, too.)

@corneadoug
Copy link
Contributor

@swkimme Since we moved to Apache Foundation, we should probably move this PR, however I was thinking of doing a bit a code cleaning, especially since GSOC2015 will start soon, and we may have somebody working on the Visualization part. (currently the .js code is really messy, I guess you know it since you split some in this PR)

There also has been quite some changes since we started this PR, and lot of code to transform in classes anyway, as well as a bit of research to do.

What do you say, I start refactoring on basic AngularJS on the Apache Foundation git. And then we recreate a PR to switch to typescript?

@swkimme
Copy link
Contributor

swkimme commented Apr 3, 2015

Good! I guess you're an expert on frontend, I'll agree whatever you're
going to do. Are you going to create another PR by yourself?

On Fri, Apr 3, 2015 at 10:49 AM CORNEAU Damien notifications@github.com
wrote:

@swkimme https://github.com/swkimme Since we moved to Apache
Foundation, we should probably move this PR, however I was thinking of
doing a bit a code cleaning, especially since GSOC2015 will start soon, and
we may have somebody working on the Visualization part. (currently the .js
code is really messy, I guess you know it since you split some in this PR)

There also has been quite some changes since we started this PR, and lot
of code to transform in classes anyway, as well as a bit of research to do.

What do you say, I start refactoring on basic AngularJS on the Apache
Foundation git. And then we recreate a PR to switch to typescript?


Reply to this email directly or view it on GitHub
#321 (comment).

@corneadoug
Copy link
Contributor

I will create a PR for basic angularjs code refactoring, I will use some of the modifications you did in this PR. Once it's done, let's create a PR for typescript. I will setup the folder structure and front-end build tools, then we can work together to migrate code to typescript (Since there will be a lot of classes to do)

@swkimme
Copy link
Contributor

swkimme commented Apr 3, 2015

Agreed.
I'm waiting for your setup, tell me if you need any help!

On 2015년 4월 3일 (금) 12:04 CORNEAU Damien notifications@github.com wrote:

I will create a PR for basic angularjs code refactoring, I will use some
of the modifications you did in this PR. Once it's done, let's create a PR
for typescript. I will setup the folder structure and front-end build
tools, then we can work together to migrate code to typescript (Since there
will be a lot of classes to do)


Reply to this email directly or view it on GitHub
#321 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants