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
Windows performance:compare #2450
Conversation
1ae9fcd
to
9235af4
Compare
a58e52c
to
e0dbd6e
Compare
e0dbd6e
to
3521e3f
Compare
Now ready |
f7a9eac
to
0f480dc
Compare
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.
@janbiedermann thanks for this PR! Looks great, just left a couple of comments for minor changes, can you please try them on windows?
Meanwhile I'm pre-approving 👍
@@ -15,7 +15,7 @@ module Util | |||
# @param str [String] string to minify | |||
# @return [String] | |||
def uglify(source, mangle: false) | |||
sh "bin/yarn -s run terser -c #{'-m' if mangle}", data: source | |||
sh "#{'ruby ' if Gem.win_platform?}bin/yarn -s run terser -c #{'-m' if mangle}", data: source |
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.
nit: we might want to use Gem.ruby
here, but not necessary
require_relative "#{__dir__}/../lib/opal/util" | ||
require_relative "#{__dir__}/../lib/opal/os" |
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.
nit: the initial "#{__dir__}/
might not be necessary
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 had to use dir so that it would use the patched version
@@ -1,4 +1,8 @@ | |||
require_relative "#{__dir__}/../lib/opal/os" |
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.
nit: also here using dir shouldn't be necessary
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 had to use dir so that it would use the patched version
@hmdne OS abstraction is working great |
No description provided.