Skip to content

Module render performance tweak - #823

Merged
TrySound merged 1 commit into
masterfrom
perf-tweak-1
Aug 5, 2016
Merged

Module render performance tweak#823
TrySound merged 1 commit into
masterfrom
perf-tweak-1

Conversation

@TrySound

@TrySound TrySound commented Aug 2, 2016

Copy link
Copy Markdown
Member

Ref #723

@TrySound

TrySound commented Aug 2, 2016

Copy link
Copy Markdown
Member Author

/cc @Rich-Harris Do you think it's safe?

@TrySound
TrySound merged commit 687b24b into master Aug 5, 2016
@TrySound
TrySound deleted the perf-tweak-1 branch August 7, 2016 09:32
@ericf

ericf commented Aug 15, 2016

Copy link
Copy Markdown
Contributor

This was not safe, and started breaking my build.

Previously with v0.34.3 my format: 'es' build worked correctly, then it started failing after this change. It was changing React.createElement to React__default.createElement but never defining React__default.

An example of the issue is here: https://travis-ci.org/yahoo/react-intl/jobs/150944243#L1425

I kept installing all the recent versions until things broke, and narrowed it down to this change.

@TrySound

Copy link
Copy Markdown
Member Author

@ericf Thank you for report. Will be fixed ASAP.

@TrySound

Copy link
Copy Markdown
Member Author

@ericf Can you produce some minimal test case? Can't figure out how to build your repo on windows.

@ericf

ericf commented Aug 19, 2016

Copy link
Copy Markdown
Contributor

@TrySound I figured out the issue, and it makes sense why this change would cause it. I am calling rollup() once, but calling bundle() twice. I created this script before the targets feature was added: https://github.com/yahoo/react-intl/blob/master/scripts/build-lib.js#L31-L41

I will update my script to be more correct to fix this.

@yairEO

yairEO commented Jan 10, 2017

Copy link
Copy Markdown

@ericf - well? did the merge break your code or all is good?

@Rich-Harris

Copy link
Copy Markdown
Contributor

@yairEO the change in this PR was undone in 1d7c853

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.

4 participants