-
Notifications
You must be signed in to change notification settings - Fork 72
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
Transpiling #9
Comments
A reasonable idea, but since we don't use this in the browser, not something that I'll have the time for in-house. I've given the issue the "contribution welcome" label. |
alternatively, changing the following two lines
to:
appears to obviate the need for transpilation. |
Well... ES5 is pretty old at this point. I've been the first to insist on
keeping compatibility with it on many things but it's really getting to the
point where those who wish to support it should be running their imports
through babel or something.
…On Mon, Aug 27, 2018 at 9:56 AM, Marty Jones ***@***.***> wrote:
alternatively, changing the following two lines
let rightSize = false;
let wordUsed;
to:
var rightSize = false;
var wordUsed;
appears to obviate the need for transpilation.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#9 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAB9fZBMqzTHz6sk-8ftRfk7g6B83B14ks5uU_p7gaJpZM4WMaJt>
.
--
*THOMAS BOUTELL, CHIEF SOFTWARE ARCHITECT*
P'UNK AVENUE | (215) 755-1330 | punkave.com
|
@murtyjones Took the liberty to do a full ES6 rewrite, added Babel while at it. Can you check this branch does the trick? |
Thanks for giving it a go.
I have concerns though. I still see lots of "var", not sure what was
accomplished by the rewrite. "var fnName = function fnName" is an
unnecessary construct and it breaks top-down programming, which has never
stopped being a best practice. I think readable code got replaced with a
less readable reshuffling and I don't see any particular improvement as a
result.
On a more minor note, eslintrc extends something that sounds in-house to me:
module.exports = { extends: "airbnb", parser: "babel-eslint"};
…On Tue, Oct 2, 2018 at 9:05 AM Carl H. ***@***.***> wrote:
@murtyjones <https://github.com/murtyjones> Took the liberty to do a full
ES6 rewrite, added Babel while at it. Can you check this branch
<https://github.com/CrlH/random-words/tree/feature/es6-rewrite> does the
trick?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#9 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAB9fX3UqQCmN5M3hp7p0_v1v0i2Dtrjks5ug2SggaJpZM4WMaJt>
.
--
*Thomas Boutell, Chief Software Architect*
P'unk Avenue | (215) 755-1330 | punkave.com
|
I've forked this library so I'm going to go ahead and unsubscribe from this issue, after noting the following:
|
@murtyjones Indeed, but since it can be used in non-browser environment, I rewrote to ES6 anyway, transpiling it. Overkill maybe, I just liked the idea. :-) Regarding the code:
|
Use of the airbnb config is fine as long as it's something that the dependencies are enough to install/discover/whatever; that's what I'm asking about. I was looking in the wrong place, yes. Still not sure why using "const" for functions and thus requiring them to come before the higher-level functions that call them is an improvement, though. Fork it if you want I guess (not that I can stop you (: ) |
Nice library.
FYI, its usage in browsers is hampered by the fact that there are some things needed transpile in older browser (
let
was the main thing I noticed).I got around it by manually specifying transpiling in my webpack config, but you might consider adding some transpiling at the module level.
The text was updated successfully, but these errors were encountered: