Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Re-enable babel-plugin-transform-react-constant-elements when it’s not buggy #553

Closed
gaearon opened this issue Sep 2, 2016 · 24 comments
Closed

Re-enable babel-plugin-transform-react-constant-elements when it’s not buggy #553

gaearon opened this issue Sep 2, 2016 · 24 comments
Labels
issue: bug tag: underlying tools

Comments

@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2016

See #525 for the reference.
@gaearon gaearon added this to the 1.0.0 milestone Sep 24, 2016
@gaearon
Copy link
Contributor Author

gaearon commented Sep 24, 2016 •
edited

Here’s a list of bugs I found by searching. I think it’s on us to triage and fix them.

transform-react-constant-elements

transform-react-inline-elements

@DrewML
Copy link

DrewML commented Oct 23, 2016

Friendly update: I have a PR in to fix babel/babel#3728. Additionally, babel/babel#4455 doesn't appear to be an issue in the current version (based on my testing with the info provided in the issue). I'll be working on babel/babel#4027 pretty soon so that we can get transform-react-inline-element back into a usable state for y'all.

Once those are done, I'll start trying to look into the pile of react-constant-elements bugs 😑

@gaearon
Copy link
Contributor Author

gaearon commented Oct 23, 2016

Wow, thank you!

@DrewML
Copy link

DrewML commented Oct 23, 2016

Alright, have a PR in for babel/babel#4027 now, too. Once those 2 PRs get merged, you should be ready to re-enable that plugin 😸

@DrewML
Copy link

DrewML commented Oct 24, 2016 •
edited

These one's can be checked off for transform-react-constant-elements:

It turns out Sebastian fixed these here in July, and the issues just never got closed out.

Few more to go 😃

@DrewML
Copy link

DrewML commented Oct 24, 2016

OK, after the release today, all the bugs related to transform-react-inline-elements (that we know of) have been addressed. Please reach out if you see more once it is re-enabled.

@gaearon gaearon mentioned this issue Nov 25, 2016
Closed
@appden
Copy link

appden commented Dec 4, 2016

FYI: I just submitted PR babel/babel#4940 that fixes babel/babel#4419 and babel/babel#4804 (which should be added to the list above, and babel/babel#4458 should be updated as closed).

There's an existing PR (babel/babel#4787) to address babel/babel#4397, but it looks stalled on implementation details.

@Timer
Copy link
Contributor

Timer commented Dec 4, 2016 •
edited

Neat, @appden! I've added babel/babel#4804 to the list.

I don't want to check off babel/babel#4458 (yet) because it seems to have been "magically" fixed without a trace of where the regression was introduced or resolved. We can save that one for last and determine if we want to let it slide.
I would love to see a test case added which prevents future regressions for that specific issue.
I'm not sure on other's opinions about this.

@gaearon gaearon mentioned this issue Dec 6, 2016
Closed
@layershifter layershifter mentioned this issue Jan 9, 2017
Merged
4 tasks
@insin insin mentioned this issue Jan 14, 2017
Open
@levithomason levithomason mentioned this issue Jan 16, 2017
Closed
@slorber
Copy link

slorber commented Jan 17, 2017

@gaearon you might want to track this one too: babel/babel#5149

@gaearon
Copy link
Contributor Author

gaearon commented Jan 17, 2017

Thanks, added to the list.

@gaearon
Copy link
Contributor Author

gaearon commented Feb 13, 2017

And... we're good? 🤗

@gaearon gaearon modified the milestones: 0.10.0, 1.0.0 Feb 13, 2017
@gaearon
Copy link
Contributor Author

gaearon commented Feb 13, 2017

With next Babel release that is.

@existentialism existentialism mentioned this issue Feb 14, 2017
Merged
Timer pushed a commit that referenced this issue Feb 15, 2017
@existentialism @Timer
Re-enable transform-react-constant-elements (#1552)
9c33f60 
* Bump babel deps

* Re-enable transform-react-constant-elements

Resolves #553
@slorber
Copy link

slorber commented Feb 16, 2017

@gaearon there might be other bugs discovered:

Maybe it's worth waiting a bit for community feedback after the fixes, and only reactivate this in CRA when it looks stable?

@gaearon gaearon reopened this Feb 16, 2017
@gaearon gaearon mentioned this issue Feb 16, 2017
Merged
@gaearon
Copy link
Contributor Author

gaearon commented Feb 16, 2017

👍

danielfigueiredo pushed a commit to danielfigueiredo/create-react-app that referenced this issue Feb 22, 2017
@existentialism
Re-enable transform-react-constant-elements (facebook#1552)
348522a 
* Bump babel deps

* Re-enable transform-react-constant-elements

Resolves facebook#553
kst404 pushed a commit to kst404/e8e-react-scripts that referenced this issue Mar 2, 2017
@existentialism @Timer
Re-enable transform-react-constant-elements (facebook#1552)
fc49fec 
* Bump babel deps

* Re-enable transform-react-constant-elements

Resolves facebook#553
@brantphoto
Copy link

left a comment in babel/babel/issues/5325, for me I had to make sure I to make sure that babel-plugin-transform-react-constant-elements was the last plugin listed in order for it to work without error

@slorber
Copy link

slorber commented Apr 6, 2017

Adding the constant-elements plugin last solved the problem for me too

@gaearon
Copy link
Contributor Author

gaearon commented Apr 6, 2017

Wow, thanks for the info. Seems like we have to wait for 7.0 for babel/babel#5415 though.

@slorber
Copy link

slorber commented Apr 6, 2017

Some other issues related to the plugin have been opened since: https://github.com/babel/babel/search?q=transform-react-constant-elements&type=Issues&utf8=%E2%9C%93

@gaearon gaearon modified the milestones: 0.11.0, 0.10.0 May 8, 2017
@gaearon
Copy link
Contributor Author

gaearon commented May 8, 2017

Postponing as issues just keep coming up. 😞
I’m not sure we’ll ever enable it.

@tarjei
Copy link

tarjei commented May 8, 2017 via email

@gaearon gaearon mentioned this issue Jul 6, 2017
Closed
@levithomason
Copy link

Looks like the last two unchecked items on the list are closed issues:

babel/babel#5325
babel/babel#5315

Are we good to go here now?

@gaearon
Copy link
Contributor Author

gaearon commented Aug 28, 2017

I think they're waiting for Babel 7.

JayCanuck added a commit to enactjs/cli that referenced this issue Oct 9, 2017
@JayCanuck
Removed babel-plugin-syntax-dynamic-import as it's already included v…
d2e9e55 
…ia stage-0 preset.

Removed babel-plugin-transform-react-constant-elements due to known issues (see facebook/create-react-app#553)
@JayCanuck JayCanuck mentioned this issue Oct 10, 2017
Merged
aarontam pushed a commit to enactjs/cli that referenced this issue Oct 30, 2017
@JayCanuck @aarontam
PLAT-40726 & ENYO-4798: Support babel-preset-env with dynamic targett…
53537aa 
…ed babel-polyfill usage (#102)

* Removed babel-plugin-syntax-dynamic-import as it's already included via stage-0 preset.
Removed babel-plugin-transform-react-constant-elements due to known issues (see facebook/create-react-app#553)

* PLAT-40726: Support babel-preset-env with dynamic targetted babel-polyfill usage.

* ENYO-4798: Babel improvements via additional plugins

* Remove blank comment line

* Exclude transform-regenerator to avoid regenerator runtime as that's not a supported feature anyway.

* Disable web.dom.iterable polyfill as its window/DOM-related and not a strict js polyfill. Unneeded for context of Enact app.

* Remove webOS build option. Instead, for webOS,set target browser or BROWSERSLIST env var to "Chrome 53"

* Remove webOS alias option


Reviewed-By: Roy Sutton (roy.sutton@lge.com)
Integrated-By: Aaron Tam (aaron.tam@lge.com)
@gaearon
Copy link
Contributor Author

gaearon commented Jan 20, 2018

Honestly I don't see this happening. New bugs keep reappearing, and I expect this will continue to be true unless Babel's architecture gets overhauled. The plugin itself is also not the best solution because it moves the cost to initialization time, potentially hurting the TTI. An ideal solution would only initialize what's necessary on demand.

I think we won't proceed with this. In the future (one-two years) I expect we can revisit a similar optimization utilizing Prepack instead.

@gaearon gaearon closed this as completed Jan 20, 2018
@gaearon gaearon removed this from the 2.0.0 milestone Jan 20, 2018
@andregoncalvesdev andregoncalvesdev mentioned this issue Mar 22, 2018
Merged
@ggregoire
Copy link

What about babel-plugin-transform-react-inline-elements? Any reason to not enabling this plugin?

@kurtextrem kurtextrem mentioned this issue Apr 27, 2018
Open
@imagine10255 imagine10255 mentioned this issue Dec 11, 2018
Closed
@lock lock bot locked and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
issue: bug tag: underlying tools
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@appden @tarjei @ggregoire @Timer @slorber @gaearon @levithomason @DrewML @brantphoto