-
Notifications
You must be signed in to change notification settings - Fork 1
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
Pattern Matching #22
base: master
Are you sure you want to change the base?
Pattern Matching #22
Conversation
@@ -11,6 +11,27 @@ export const lightscriptImports = { | |||
"bitwiseZeroFillRightShift": "inline", | |||
}; | |||
|
|||
export const runtimeHelpers = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I liked this way of doing helpers a lot, so I tried to adopt it. When I applied it to my local build of lscdiag
I got some very strange code output in the generated helpers. Took me a while to figure out why -- turns out that minifiers mangle these things and then fn.toString()
brings in the minified code.
We could just say we don't care about that, but I think live compilers and minifiers are good things generally, whereas this, while neat, could just be replaced with a plain string passed to babel-template. I recommend the latter approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, yeah, that's why. I kind of don't mind – it's babel helping make sure that I'm writing cross-browser code. But yes, if you see the tests, the output isn't the exact same as what I wrote.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't sound quite right... the transform being run here is just the one specified in .babelrc
for the plugin, which is just env: { node: 4 }
-- that's definitely not cross browser code.
I think having cross-browser code is a good point, but might be better to paste the helper into babel-repl, set the env to Netscape Navigator 1.0, and paste the output into a plain string.
Alternatively, an extra build step could compile the helpers with a different env? But that seems like overtooling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My current implementation of hasProps
and hasLength
doesn't change due to this factor, which I think is desirable, so I think I'll leave this as-is for now, hope that's ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I hadn't read that comment very carefully – I did check in the babel repl that with an ie<6 target the output doesn't change
(travis is failing because the parser has not yet merged) |
builds on parser PR at lightscript/babylon-lightscript#17