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

update (Atma.js) rewrite the app #1337

Merged
merged 1 commit into from Sep 12, 2015
Merged

Conversation

tenbits
Copy link
Contributor

@tenbits tenbits commented Jun 11, 2015

Hello Team,

updating here the Atma.js example with latest dependencies and features.

Thank you,
Alex

@arthurvr
Copy link
Member

Thanks for doing this!

2. Hint: Viewing *.mask files enable javascript or less syntax highlighting in your IDE
1. Read the readme.md - you will get basic information about the libraries
2. Hint: Use Sublime/Atom plugins to highlight the *.mask syntax
, view them on github or enable Javascript/Less syntax for the extensions
Copy link
Member

Choose a reason for hiding this comment

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

The positioning of this comma looks a little weird. Let's just keep comments as we'd write all-day sentences.

@tenbits
Copy link
Contributor Author

tenbits commented Jun 11, 2015

Hey Arthur, thanks for the feedback, and sorry, my bad - unit tests have run upon the PR, but the jscs have forgotten. Hopefully, haven't missed anything now.

Cheers,
Alex

ps. The quotes in mask syntax, like in js object literals, are not required.

-->

<script type="text/mask" data-run="auto" >
Copy link
Member

Choose a reason for hiding this comment

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

Seems like there's a space too much before >.

@tenbits tenbits force-pushed the atmajs-update branch 2 times, most recently from 3bee849 to 1e16329 Compare June 11, 2015 19:14
@tenbits
Copy link
Contributor Author

tenbits commented Jun 11, 2015

@arthurvr, thank you for your comments. The style errors are gone, but it seems, export CHROME_BIN=chromium-browser is missed in .travis.yml

1. Read readme.md - you will get basic information about the libraries
2. Hint: Viewing *.mask files enable javascript or less syntax highlighting in your IDE
1. Read the readme.md - you will get basic information about the libraries
2. Hint: Use Sublime/Atom plugins to highlight the *.mask syntax,
Copy link
Member

Choose a reason for hiding this comment

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

Use Sublime/Atom plugins to highlight the *.mask syntax,

Seems like more somthing for the readme than for comments here. In fact I think this whole comment block would fit better in the readme.

@arthurvr
Copy link
Member

@tenbits you shouldn't care about the CI for now. Just run the tests locally for the time being. I just ran them locally and seems like everything passes 🍰

The style errors are gone,

Seems like there are still some missing newlines.

"todomvc-app-css": "^1.0.0"
"maskjs": "^0.51.26",
"ruta": "^0.1.16",
"todomvc-app-css": "^1.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Now that we're rewriting anyways, mind switching to 2.0? You'll need to update from ids to classes. It will make all tests fail but don't worry, we're on it. Thanks!

@arthurvr
Copy link
Member

Any community member(s) that want to review this? We want to make sure it follows best practices but I'm not the right one to review the code myself.

@tenbits
Copy link
Contributor Author

tenbits commented Jun 14, 2015

Arthur, now the application uses todomvc-app-css@2.0.1. Also I have moved some of the index.html comments to the readme. The application was developed with the frameworks authors, so it should represent best practicies for the last versions.

@arthurvr
Copy link
Member

Should this example now move to the compile-to-js section?

@@ -109,10 +116,9 @@ navigate to ``` http://localhost:5777/ ```

### Build

To build the application for release, run ``` $ atma build --file index.html --output release/```. We provide also a compiled version in 'build/' directory, so you
can see how the application looks like for production.
To build the application for release, run ``` $ atma build --file index.html --output release/```.
Copy link
Member

Choose a reason for hiding this comment

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

Use a single backtick for inline code.

@arthurvr
Copy link
Member

Thanks for your hard work on this!

@tenbits
Copy link
Contributor Author

tenbits commented Jun 14, 2015

Should this example now move to the compile-to-js section?

I wouldn't do this, as mask is still the template engine, which is parsed at runtime to the MaskDOM. One can use also html syntax to declare the template. The build process only combines the templates to one resource.

Use a single backtick for inline code.
mind expanding this to --global?
GitHub
this alignment is inconsistent
please use your full name

Done

Thanks for your hard work on this!

And I take off my hat to you.

@arthurvr
Copy link
Member

@tastejs/todomvc Anyone of us who wants to take an extra look? If not I'm going to merge this one shortly.

@samccone
Copy link
Member

This has a memory leak that is creating detached DOM nodes that we should try and fix before we merge

Steps to reproduce
  • start timeline profiler
    • add three todos
    • delete three todos
    • (repeat above 2 more times)
  • force GC
  • stop timeline profiler

screen shot 2015-06-17 at 10 19 30 am
screen shot 2015-06-17 at 10 18 57 am

@tenbits
Copy link
Contributor Author

tenbits commented Jun 19, 2015

The one thing regarding the detached nodes, which dont let me sleep :) :
I run the test script, add/remove a todo 50 times. And see 7 detached nodes (other detached nodes are for the internal use). But these, 7, are produced by a todo item.

snapshot1

Then I manually create one more todo and see the 7 detached nodes to disappear.

snapshot2

Something strange happens somewhere ) I'll look later in details.
Cheers, and thank you once again.

@tenbits
Copy link
Contributor Author

tenbits commented Jun 19, 2015

got

Caught them) Shortly sad: a BinderFactory (Singleton) holds the last binded TextNode. And this TextNode was the part of the detached tree, so after removal of the last Todo, BinderFactory still holds the reference, but after we create a new Todo, the reference is replaced with the new one, and GC now collects the detached DOM tree. There is a little bit complicated binding process, as the rendering should work also in Node.js with further bootstrapping on the client.

Anyway, a patch was applied to the binder library, and now you shouldn't see those detached nodes.

Sam, once again, a HUGE thanks to you for pointing me at this!!


— is not only an url routing via History API or ```hashchange```, but it implements a Route-Value Collection for adding/retrieving any object by the route.

#### Atma.Toolkit
[github](https://github.com/atmajs/Atma.Toolkit)
[GitHub](https://github.com/atmajs/Atma.Toolkit)

— command-line tool, which runs unit tests, builds applications, runs node.js ```bash``` scripts, creates static file server with live reload feature, etc.
Copy link
Member

Choose a reason for hiding this comment

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

Node.js (capital N). Also only use one backtick for bash.

@tenbits
Copy link
Contributor Author

tenbits commented Jul 2, 2015

Node.js (capital N). Also only use one backtick for bash
one backtick for inline code

Thanks, Arthur, for helping me with this!

@arthurvr
Copy link
Member

arthurvr commented Jul 2, 2015

Thank you!

@arthurvr
Copy link
Member

Can you rebase this branch upon master and run the tests? We recently updated our test suite to work with todomvc-app-css 2.0 😸

@tenbits tenbits force-pushed the atmajs-update branch 4 times, most recently from 74483d8 to de84dea Compare July 12, 2015 22:29
@tenbits
Copy link
Contributor Author

tenbits commented Jul 12, 2015

✔️ rebased

@arthurvr
Copy link
Member

Thanks! Looks like tests pass 🎉

@samccone
Copy link
Member

going to do one pass of leaky on this then ⛵

});
};

}());
Copy link
Member

Choose a reason for hiding this comment

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

})(); for consistency with the app template.

@samccone
Copy link
Member

oh :(

Looks like we still have a pretty significant leak of listeners

initial snapshot: {"documents":3,"jsEventListeners":13,"jsHeapSizeUsed":6700408,"nodes":203}
end snapshot:     {"documents":3,"jsEventListeners":43,"jsHeapSizeUsed":9005448,"nodes":353}

@tenbits
Copy link
Contributor Author

tenbits commented Jul 13, 2015

Yeap, Sam, after the leaky test run I can confirm the same behaviour 😞 Before last rebase I have removed the jquery dependency, seems something went wrong here. I'll dig into this.

@samccone
Copy link
Member

ok, thanks so much for your persistence on this front 👏

@tenbits
Copy link
Contributor Author

tenbits commented Jul 13, 2015

Sam, may I ask, what Chrome version are you testing upon?
My default Chrome version is 43, and I get the same result as yours, but when I add Chrome Canary Path in https://github.com/samccone/todomvc/blob/sjs/leak/tests/leaks.js#L7 there is no listener leaking:

Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.132 Safari/537.36
initial snapshot: {"documents":3,"jsEventListeners":13,"jsHeapSizeUsed":4079668,"nodes":203} 
end snapshot:     {"documents":3,"jsEventListeners":43,"jsHeapSizeUsed":5271932,"nodes":353}

Mozilla/5.0 (Windows NT 6.3; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/45.0.2454.5 Safari/537.36
initial snapshot: {"documents":3,"jsEventListeners":12,"jsHeapSizeUsed":6920560,"nodes":196} 
end snapshot:     {"documents":3,"jsEventListeners":12,"jsHeapSizeUsed":8275360,"nodes":226}

@samccone
Copy link
Member

i was testing this on 43, but let me run it against canary

@tenbits
Copy link
Contributor Author

tenbits commented Jul 13, 2015

It seems, this is the same issue as before with input[checkbox]. And as the snapshots don't show any leaks, I assume this is only the counter issue, and not the real leaks. I was also wondering, why React shows always constant listeners count, but it turned out, React doesn't bind listeners directly to elements, but uses global document listeners (delegations).

@tenbits
Copy link
Contributor Author

tenbits commented Jul 14, 2015

Thanks Arthur, I have applied the changes.

@samccone
Copy link
Member

Just ran my latest memory profile iteration over this change set and it looks like we are good to go!

samccone added a commit that referenced this pull request Sep 12, 2015
update (Atma.js) rewrite the app
@samccone samccone merged commit 987bc94 into tastejs:master Sep 12, 2015
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