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

Rewrite in ES6 using webpack #511

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

Rewrite in ES6 using webpack #511

wants to merge 59 commits into from

Conversation

mikeric
Copy link
Owner

@mikeric mikeric commented Aug 3, 2015

This branch converts the codebase from CoffeeScript to ES6, and replaces previous gulp tasks with webpack only. It still needs a lot of attention before it can be considered stable and merged in.

Todo

  • Address all failing specs (17 at time of writing).
  • Test throughly for regressions. This may require a bit of backtracking and documenting features that have snuck in without being fully documented.
  • Write unit tests around any undocumented behaviour that was discovered during above testing.
  • Add multiple compile targets with webpack like we had before:
    • rivets.bundled.js (sightglass included, unminified)
    • rivets.bundled.min.js (sightglass included, minified)
    • rivets.js (standalone, unminified)
    • rivets.min.js (standalone, minified)

(Added by @Leeds-eBooks on 23 Aug 15):

  • Work through pull requests and feature requests.

(Added by @Leeds-eBooks on 26 Jan 16):

  • Specific fixes:
    • Regex to handle pipes in formatter argument strings

@benadamstyles
Copy link
Collaborator

I'm trying to address the Rivets.TextTemplateParser is undefined errors. It seems that TextTemplateParser.parse() has been replaced by parseTemplate(), but this is now exposed as an ES6 module rather than a property on the global rivets._ object. Is that correct? This means it can't be tested in the same way – we either need to import it into the unit test, which means having some sort of require mechanism in the mocha test runner html page, or parse the template using rivets as a public user would, but I'm not sure how to go about that. Hope I'm making sense...

@benadamstyles
Copy link
Collaborator

After spending some more time with the code, I think we have 2 options: add an extra property to the rivets factory in export.js that can expose private functions for testing (something like rivets._ = {parseTemplate}), or use babel in the test runner so we can import private functions for testing. My inclination is for the latter. @mikeric , @Duder-onomy , thoughts?

@benadamstyles
Copy link
Collaborator

Having a hard time researching how to elegantly integrate a babel compiler as part of the mocha-phantomjs test script. This would be a lot easier with Karma, I feel. Reckon that's an option? Or do you know a better way to use es6 in our tests with mocha?

@MiguelCastillo
Copy link
Contributor

@Leeds-eBooks sorry to barge in. I use karma with mocha for ES6/babel in projects at work. For my side projects I use a module loader I created that you can hook babel directly into, and run in the browser/phantomjs, which runs mocha directly; no Karma.

Karma is a good alternative.

@benadamstyles
Copy link
Collaborator

@MiguelCastillo Your module loader sounds intriguing. Is it public?

@MiguelCastillo
Copy link
Contributor

Yeah, it is public. https://github.com/MiguelCastillo/bit-imports.

The example is all written in ES2015 and transpiled in the browser. I can perhaps create a quick scaffold for bit-imports/babel/mocha, if you are interested.

@MiguelCastillo
Copy link
Contributor

Here are some unit tests using with bit-imports/mocha that run in the browser/phantomjs... https://github.com/MiguelCastillo/bit-imports/tree/master/test
https://github.com/MiguelCastillo/bit-loader/tree/master/test

If you want to see a unit test setup with multiple module types running, check this one out.
https://github.com/MiguelCastillo/bit-sandbox

@benadamstyles
Copy link
Collaborator

@MiguelCastillo so how would we go about setting this up for the rivets tests? If I understand your docs correctly, we load bit-imports in a regular script tag in the runner.html page, then System.import() our test js files and the rivets es6 modules we want to test directly?

@MiguelCastillo
Copy link
Contributor

@Leeds-eBooks Yup, broad strokes that's correct. Load bit-imports and System.import specrunner. There are a few steps in between that I am writing a recipe for. I will send out a link a little later when it's ready.

In the meantime, if you want to see a setup that's running this unit test setup, take a look here. No instruction on what the moving pieces are, and that's what I am writing as a recipe for you today.

If I have time today, I will fork rivets and do a setup in for this branch.

  1. index.html has mocha loaded via script tag.
  2. polyfill promises if needed.
  3. load bit imports via script tag.
  4. load a config file script to setup bit-imports.
  5. setup mocha.
  6. System.import unit tests.

@benadamstyles
Copy link
Collaborator

Hi everyone, I've just pushed a commit that gets the parseTemplate module function into the global scope so it can be tested. It's not the prettiest solution but I haven't had time to get my head around the bit-imports concept (sorry @MiguelCastillo :/ ) and as it's only a problem with one small part of the tests I don't think it's something any of us should spend a lot of time on. I am using an npm pretest script to call babel (as a global npm package) on the src/parsers.js file and put the transpiled output in spec/lib/parsers.js, it works, the tests pass. If anyone would like to make this more elegant please do!

There are now just 3 tests failing. They have me stumped, for now.

@benadamstyles
Copy link
Collaborator

I have added some logs to my local repo to try and work out what's going with the rv-each binder. This is what I'm getting:

it("lets you push an item", function() {
  var view = rivets.bind(fragment, model);
  var originalLength = model.items.length;

  // one child for each element in the model plus 1 for the comment placeholder
  Should(fragment.childNodes.length).be.exactly(model.items.length + 1);

  console.log(model.items.length); // LOGS: 3
  console.log(model.items.map(function(x) {return typeof x + " " + x;})); // LOGS: ["object [object Object]","object [object Object]","object [object Object]"]
  model.items.push({"val": 3});
  console.log(model.items.map(function(x) {return typeof x + " " + x;})); // LOGS: ["object [object Object]","object [object Object]","object [object Object]","object [object Object],[object Object],[object Object],,push","string push"]
  console.log(model.items.length); // LOGS: 5

  Should(model.items.length).be.exactly(originalLength + 1);
  Should(fragment.childNodes.length).be.exactly(model.items.length + 1);
});

So after push({"val": 3}), the array gains 2 new elements – an array which seems to be a copy of the parent array itself, and a string "push". @mikeric Can you shed any light on this? I've tried with no success to find the source of this behaviour. Thanks!

@jhnns
Copy link
Contributor

jhnns commented Aug 14, 2015

If you feel that this branch is ready to merge, I can install this branch in a big project to check if there are any breaking changes.

@benadamstyles
Copy link
Collaborator

@jhnns Thanks man, but until all tests are passed, I don't think it's worth it, as they are testing for basic functionality and failing tests to me mean a broken rivets. Do you have any insight into this specific problem? There are only 3 tests that we still failing and I'm banging my head against a brick wall trying to fix them... There's something weird going on with the each binder and mutation of the input array but I can't trace the source of the issue.

@ydaniv
Copy link

ydaniv commented Aug 15, 2015

@Leeds-eBooks I'm suspecting line 56 in adapter.js is misbehaving. This is the line in charge of pushing the elements to the array. Since the wrapping function is an arrow function you can not use the arguments object there. So probably that's your culprit.

@benadamstyles
Copy link
Collaborator

@ydaniv Ah thanks man, but I spotted that one and it doesn't fix the problem. Weird that it wasn't causing any failing tests. Anyway I'll push that fix today. But still completely stuck on this!

benadamstyles and others added 4 commits August 15, 2015 09:57
…eves the incorrect context, and therfore had no observer when the binding publishes.
…eves the incorrect context, and therefore had no observer when the binding publishes.
@benadamstyles
Copy link
Collaborator

@Duder-onomy I'm still getting 3 failing tests, under Rivets.binders each-* and under Functional input.

EDIT: forgot to run build :P great work!

benadamstyles and others added 30 commits February 4, 2016 13:26
…_ES6

Report regexp to split formatters. Fixed #432
Report on ES6 branch : generated index for nested rv-each. Docs included
Don't use view.els.forEach to avoid exception when jquery is used
Fixed binding to allow multiple strings, ES6 version of #624 Fixes #620
Got ES6 Travis build working + Updated Webpack & Babel
…d-attributes

Fix/camelcase components dashed attributes
…tribute-ES6

Report Fix uncorrect '0' receive by binders if html attribute is not …
…s to be already effective on ES6 branch
Report fix for #644 on ES6. Only the test has been reported, fix seem…
Report #619. It's only a test, actual fix is on sightglass 0.2.6
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

10 participants