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

The new tmpl does not pass all the riot tests #1

Closed
GianlucaGuarini opened this issue Aug 18, 2015 · 6 comments
Closed

The new tmpl does not pass all the riot tests #1

GianlucaGuarini opened this issue Aug 18, 2015 · 6 comments
Assignees

Comments

@GianlucaGuarini
Copy link
Member

@aMarCruz
Including the newest version of tmpl.js some riot tests fail. Could you please fix the issue?
Steps to reproduce the issue:

  • switch to the riot 2.3.0 branch
  • install npm install riot-tmpl
  • include it in riot
import 'browser/wrap/prefix'
import 'browser/global-variables'
import '../node_modules/riot-observable/lib/index'
riot.observable = observable
import 'browser/mixin'
import 'browser/router'
import '../node_modules/riot-tmpl/lib/index'
import 'browser/tag/mkdom'
import 'browser/tag/each'
import 'browser/tag/parse'
import 'browser/tag/tag'
import 'browser/tag/update'
import 'browser/tag/util'
import 'browser/tag/vdom'
import 'browser/wrap/suffix'
  • trigger build riot triggering the tests
@aMarCruz
Copy link
Contributor

I will

@aMarCruz aMarCruz self-assigned this Aug 19, 2015
@aMarCruz
Copy link
Contributor

@GianlucaGuarini , I push to the master brach this time, next PRs will go to the dev branch.
This version will pass all text, except:
https://github.com/riot/riot/blob/2.3.0/test/specs/tmpl.js#L133
expect(render('{ loading: !nonExistingVar.length }')).to.equal('loading')
Please comment that line, or change 'loading' to the empty string.

This change in the behavior of shorthands was for consistency with the evaluation of other expressions, now property accessors for undefined variables generate error, so that the resulting expression is blank.

There's a new folder ./src with commented code, I'm using ./rmcomms for cleanup the resulting file ./lib/index.js and conditionally include debugging code.
This is called by the Makefile, and it should be transparent to other parts of riot.

Note:
I'm disappointed with eslint 1.0, misaligned variables are poorly readable for me and sometimes is needed use extra parentheses to increase readability, so I returned to the previous version, but if there are problems I will use the new one.

@GianlucaGuarini
Copy link
Member Author

@aMarCruz well done! I am not sure we need 2 folders src and lib probably you can just use dist for the output like @cognitom does for riot-route https://github.com/riot/route/tree/master/dist

@aMarCruz
Copy link
Contributor

Thanks.
for partial tools like tmpl I think 'lib' seems appropriate, anyway, /dist is a good name.
Please review this tree:

\                  // <-- root, with copy of tmpl.js?
\src               // <-- commented source files
\dist              // <-- tmpl.js , riot-tmpl.js or index.js ? with minified versions.
\test

@GianlucaGuarini
Copy link
Member Author

I prefer the riot-route folder structure:

\lib   // <-- commented files
\dist
   tmpl.riot.js <-- no umd
   tmpl.min.js <-- minified
   tmpl.js  <-- umd wrappers

Remember also to change in package.json the path to the UMD release https://github.com/riot/route/blob/master/package.json#L5

@GianlucaGuarini
Copy link
Member Author

this issue got solved

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

No branches or pull requests

2 participants