-
Notifications
You must be signed in to change notification settings - Fork 8
Upgrade primary build target to ES2017, add browser build #28
Conversation
@@ -1,3 +1,3 @@ | |||
#!/bin/sh | |||
set -o xtrace | |||
exec tsc -p ./tsconfig.prod.json | |||
tsc -p ./tsconfig.prod.json && test -f ./tsconfig.browser.json && tsc -p ./tsconfig.browser.json |
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 think we should keep the exec
s there. Reference: #13 (review)
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.
Is the idea to silently fail when there's no browser config? If so, 👍
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.
re: exec I was having issues with it, I will dig deeper for its purpose, thanks for the link
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.
Removing the exec is right, as it means "replace the bash process with this other one". So the script will stop after tsc
executes, even if it's successful. For example, this never prints Hi:
exec true
echo "Hi"
What I'd do is set -e
at the beginning of the script. That makes the bash process fail if any of the commands fail.
@ryanio I pushed the suggested changes. Can you take another look, please? |
@evertonfraga thanks for that, looks great, just tested and seems to work as expected 👍 |
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.
Yeah, looks good, thanks Everton!
Does this mean that when VM v5 gets released it will be compiled with two targets? :) And it will include MPTv4 also with two targets? |
Yes! We already integrated it separately in MPT but it inspired this PR, so once this release gets published anyone who installs |
that's awesome! |
What is the current state of this? This needs a release I would assume and a PR on an integration in the monorepo regarding to ethereumjs/ethereumjs-monorepo#821? Is this relatively straight-forward or are there any blockers along the way? Any help needed in any regard? |
What is the state with the releases here? Can we move forward with this and integrate into the consuming libraries? |
@evertonfraga was making progress on integrating this and other ethereumjs-config@v2 changes into the monorepo, maybe he can give an update on where he is with that. i'm happy to help with any remaining work. |
This PR is a breaking change and upgrades the primary typescript build target to ES2017 and adds a ES5 browser build as inspired by ethereumjs/merkle-patricia-tree#117 and ethereumjs/ethereumjs-monorepo#603.
See the readme on how to dependent libraries should update their usage.