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

Replace babel require hook with transformFileSync #103

Closed
wants to merge 3 commits into from

Conversation

floatdrop
Copy link
Contributor

Start of discussion - 52b0ccb#commitcomment-13971327

If you have babel and want to use require hook in application code and want to run tests with ava - you can get syntax errors (SyntaxError: Unexpected reserved word) because babel will get options from ava.

@floatdrop floatdrop force-pushed the remove-require-hook branch 2 times, most recently from fc962aa to 90394d0 Compare October 25, 2015 14:02
blacklist: hasGenerators ? ['regenerator'] : [],
optional: hasGenerators ? ['asyncToGenerator'] : [],
optional: hasGenerators ? ['asyncToGenerator'] : ['runtime'],
Copy link
Member

Choose a reason for hiding this comment

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

Runtime should still be included, even if hasGenerators is true.

@sindresorhus
Copy link
Member

Does the caching still work with this solution? Since Babel is relatively slow, the caching is very important.

@floatdrop floatdrop force-pushed the remove-require-hook branch from 90394d0 to 29d4c8e Compare October 25, 2015 14:20
@floatdrop
Copy link
Contributor Author

@sindresorhus I'm afraid, cache should be implemented by hands.

co must be replaced (or patched to use Promise polyfill, since babel now will not do this for us).

@vadimdemedes
Copy link
Contributor

@floatdrop in order to support the stuff that "generator community" is used to, co is required. Babel's built-in implementation is very limited.

@sindresorhus
Copy link
Member

@floatdrop Maybe you could either extract this into a separate module we can depend on or do a PR on Babel to expose it?

@floatdrop
Copy link
Contributor Author

@vdemedes yeah, I know, but you can't use co in Node.js 0.10 without some transpiler (and I don't think, that co will adapt to it soon tj/co#244). So we need to come up with something. I believe, that transpiling only test code will improve performance compared to patched require function.

@sindresorhus okay, but need to do some performance tests - I wonder, how fast cache file will fill up and parsing time eat all the benefit.

@vadimdemedes
Copy link
Contributor

@floatdrop generators are only being used in node 0.11, iojs and node v4.x projects, so we don't have to worry about that.

@floatdrop
Copy link
Contributor Author

@vdemedes co uses global Promise object - so you can't use it right away in Node.js 0.10.

@vadimdemedes
Copy link
Contributor

@floatdrop Hmm, did not understand the point. I am just saying that nobody uses generators on the old node.js versions.

@floatdrop
Copy link
Contributor Author

@vdemedes generators are not a problem, yes. But ava do require co at the top of test.js and this leads to errors in 0.10 - https://travis-ci.org/sindresorhus/ava/jobs/87312158

@floatdrop
Copy link
Contributor Author

JFYI: seems like babel require hook have some polyfill side-effect - https://github.com/floatdrop/ava-in-node-0.10

@floatdrop
Copy link
Contributor Author

Just to keep track - waiting for tj/co#250

@sindresorhus
Copy link
Member

Doesn't look like co has much activity, so I don't think we should wait on that tbh.

@sindresorhus
Copy link
Member

Why do we even depend on co again? Can't Babel+Regenerator do this for us?

@vadimdemedes
Copy link
Contributor

Because co implements some very nice features, which babel's regenerator does not implement. Babel's implementation only makes Promises yieldable, but nothing else.

Why don't we use @floatdrop's fixed fork for now, until they merge his fix into master?

@sindresorhus
Copy link
Member

Why don't we use @floatdrop's fixed fork for now, until they merge his fix into master?

👍

@floatdrop floatdrop force-pushed the remove-require-hook branch from 3773b95 to 3f4cbfa Compare November 1, 2015 15:20
"bluebird": "^3.0.0",
"chalk": "^1.0.0",
"co": "^4.6.0",
"co": "git://github.com/floatdrop/co",
Copy link
Member

Choose a reason for hiding this comment

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

"co": "floatdrop/co#343xsv324",

use the shorthand format and use a specific commit hash (first 6 chars of the hash is enough), otherwise the npm cache will become stale.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🆗

@floatdrop floatdrop force-pushed the remove-require-hook branch from 3f4cbfa to adcf8e0 Compare November 1, 2015 19:15
@sindresorhus
Copy link
Member

okay, but need to do some performance tests - I wonder, how fast cache file will fill up and parsing time eat all the benefit.

@floatdrop I'm ok with merging this now if you commit to looking into caching later? Compilation is a lot larger overhead than just reading in a cached file from disk. I do think we can do caching better than Babel itself, though. Mainly using separate cache files for each test file (Babel uses one large file) and a LRU cache.

"bluebird": "^3.0.0",
"chalk": "^1.0.0",
"co": "^4.6.0",
"co": "floatdrop/co#343xsv324",
Copy link
Member

Choose a reason for hiding this comment

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

lol, that was a fake hash, i don't know which hash you want to use.

@floatdrop floatdrop force-pushed the remove-require-hook branch from adcf8e0 to 55d5982 Compare November 1, 2015 19:26
@floatdrop
Copy link
Contributor Author

@sindresorhus sure. I'm started to use ava in some work related projects, so I will hang around for some time :)

@sindresorhus sindresorhus mentioned this pull request Nov 1, 2015
@sindresorhus
Copy link
Member

I'm started to use ava in some work related projects

😍

@sindresorhus
Copy link
Member

This looks good to me. @vdemedes Can you take a final look?

@vadimdemedes
Copy link
Contributor

Good to go! 👍

@floatdrop floatdrop closed this in 1d5ef4c Nov 1, 2015
@sindresorhus
Copy link
Member

Thanks again @floatdrop :)

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.

3 participants