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.

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

2.5.X new project with message “React is running in production mode, but dead code elimination has not been applied” #11775

Closed
leobastiani opened this issue Nov 19, 2021 · 16 comments · Fixed by #11783

Comments

@leobastiani
Copy link

To see this error in the console you must have the React Devtools Extension
https://chrome.google.com/webstore/detail/react-developer-tools/fmkadmapgofadopljbjfkapdkoienihi?hl=en

Copy and paste this script on your terminal, you also need to have docker installed

dockerfile="FROM ubuntu:20.04
ENV DEBIAN_FRONTEND noninteractive
ENV LANG=\"C.UTF-8\"
ENV TZ=Etc/Universal
RUN ln -snf /usr/share/zoneinfo/\$TZ /etc/localtime && echo \$TZ > /etc/timezone
RUN apt-get update && \
  apt-get install -y sudo tzdata
RUN useradd -ms /bin/bash ubuntu && \
  usermod -aG sudo ubuntu
RUN echo '%sudo ALL=(ALL) NOPASSWD:ALL' >> /etc/sudoers
USER ubuntu
WORKDIR /home/ubuntu
RUN sudo apt-get install wget curl git -y
RUN sudo mkdir -p /usr/local/n && \
  sudo chown -R \$(whoami) /usr/local/n && \
  sudo mkdir -p /usr/local/bin /usr/local/lib /usr/local/include /usr/local/share && \
  sudo chown -R \$(whoami) /usr/local/bin /usr/local/lib /usr/local/include /usr/local/share
RUN wget -q -O n https://raw.githubusercontent.com/tj/n/master/bin/n && \
  bash n install 14.18.1
RUN npm install -g meteor
ENV PATH=\"/home/ubuntu/.meteor:\${PATH}\"
WORKDIR /home/ubuntu
"

echo "$dockerfile" | docker build -t meteor-ubuntu:2.5.1 -

docker run -it \
  --rm \
  -p 3000:3000 \
  --entrypoint /bin/bash \
  meteor-ubuntu:2.5.1 -c "meteor --version && \
  meteor create --typescript myapp && \
  cd myapp && \
  NODE_ENV=production meteor --production"

Open http://localhost:3000 and see that “React is running in production mode, but dead code elimination has not been applied”

I also posted it here
https://forums.meteor.com/t/meteor-2-5-1-new-project-with-message-react-is-running-in-production-mode-but-dead-code-elimination-has-not-been-applied/57031/3

@StorytellerCZ
Copy link
Collaborator

StorytellerCZ commented Nov 21, 2021

Can confirm, it started to pop up in my apps as well. I don't recall seeing it in 2.5.0 and from 2.5.1 changelog the only possible culprit is #11758. I have another more pressing bug in my app, but I'll take a look once I deal with it.

@yanickrochon
Copy link

yanickrochon commented Nov 23, 2021

Possible related StackOverflow question.

I also have this problem, and it slows down the app considerably.

@filipenevola
Copy link
Collaborator

Hi @brianlukoff, is the issue that you had with terser also happening in https://atmospherejs.com/ostrio/flow-router-extra?

Because I believe we will have to re-enable terser evaluate to solve this issue here.

The code from flow-router using var is probably making terser to interpret this code incorrectly and evaluate these local variables as constants inside that context.

@filipenevola
Copy link
Collaborator

Hi @leobastiani can you run meteor update to confirm if the new minifier-js fixes this problem for you?

@brianlukoff
Copy link
Contributor

@filipenevola Even if flow-router-extra works OK, I think we should be very hesitant to use a version of terser that we know changes the meaning of code! (If we know that terser corrupts flow-router, then what other code might it be mangling and what other subtle bugs might be introduced by its use?)

@filipenevola
Copy link
Collaborator

Hi @brianlukoff I believe the problem is not in terser but in the way the flow-router code was implemented.

We need this feature (evaluate) to correctly build React apps, for example.

Did you try to isolate the problematic code in flow-router?

I believe if you modernize this code in flow-router this problem is not going to happen anymore.

We could also revert the bump that was made in terser a few months ago but sooner or later this problem would return.

@leobastiani
Copy link
Author

Hi @leobastiani can you run meteor update to confirm if the new minifier-js fixes this problem for you?

After running meteor update it's running with no issues

dockerfile="FROM ubuntu:20.04
ENV DEBIAN_FRONTEND noninteractive
ENV LANG=\"C.UTF-8\"
ENV TZ=Etc/Universal
RUN ln -snf /usr/share/zoneinfo/\$TZ /etc/localtime && echo \$TZ > /etc/timezone
RUN apt-get update && \
  apt-get install -y sudo tzdata
RUN useradd -ms /bin/bash ubuntu && \
  usermod -aG sudo ubuntu
RUN echo '%sudo ALL=(ALL) NOPASSWD:ALL' >> /etc/sudoers
USER ubuntu
WORKDIR /home/ubuntu
RUN sudo apt-get install wget curl git -y
RUN sudo mkdir -p /usr/local/n && \
  sudo chown -R \$(whoami) /usr/local/n && \
  sudo mkdir -p /usr/local/bin /usr/local/lib /usr/local/include /usr/local/share && \
  sudo chown -R \$(whoami) /usr/local/bin /usr/local/lib /usr/local/include /usr/local/share
RUN wget -q -O n https://raw.githubusercontent.com/tj/n/master/bin/n && \
  bash n install 14.18.1
RUN npm install -g meteor
ENV PATH=\"/home/ubuntu/.meteor:\${PATH}\"
WORKDIR /home/ubuntu
"

echo "$dockerfile" | docker build -t meteor-ubuntu:2.5.1 -

docker run -it \
  --rm \
  -p 3000:3000 \
  --entrypoint /bin/bash \
  meteor-ubuntu:2.5.1 -c "meteor --version && \
  meteor create --typescript myapp && \
  cd myapp && \
  meteor update && \
  meteor --version && \
  NODE_ENV=production meteor --production"

@brianlukoff
Copy link
Contributor

@filipenevola The screenshot in #11756 is the affected code (minified) with the link to the original code. But my concern is larger than flow-router; it's that terser is changing the meaning of any code. I agree we could certainly rewrite that code in flow-router, but there is so much code that we all depend on in various libraries -- how can we be confident that terser is not also changing the meaning of other code that is not part of flow-router? Certainly we could maintain our own versions of the minifier packages to solve the problem with flow-router, but I think it's dangerous for Meteor to be automatically applying a code transformation that might change the meaning of some code that a project is relying on. (We happened to discover this bug quickly because it generated an error upon changing routes, but terser could be creating much more subtle bugs that would be harder to catch.)

I would come down on the side of reverting the update to terser until they address the issue, since other folks need evaluate to be enabled.

@filipenevola
Copy link
Collaborator

Terser has a good enough popularity IMO.

I believe if we improve flow-router code this issue is going to disappear. The code is using var and other old patterns in JS and I believe this is what caused the issue in the first place.

Meteor applies a lot of code transformations along the way, including Terser and Reify, as we have a compiler, bundler, and minifier we need to perform these operations. There is no way to avoid them.

I believe an issue in the new flow-router repo would be the best way moving forward.

@brianlukoff
Copy link
Contributor

I certainly understand that. I think updating that code in flow-router is certainly the way to go (or the newer fork may have already done so -- I haven't checked yet). But I still think it is dangerous to apply a transformation to every Meteor codebase (some may have similar legacy patterns in other older libraries) that we know has the potential to change the code's behavior and introduce bugs. Of course I am not opposed to these transformations in general -- as you said, they are critical to do -- but it's important that we can be confident that they will not change the semantics of the code!

Of course this is your call with respect to the Meteor project as a whole. But even if I switch to the newer version of flow-router I would likely still decide to maintain a local fork of minifier-js that avoids the problematic evaluate behavior in terser, because I don't want to take the risk of it potentially breaking something else.

@dr-dimitru
Copy link
Contributor

@filipenevola @brianlukoff I have fully refactored flow-router library to modern ES standard. The only place remained the same (still var used) is in tests of ostrio:flow-router-extra. Tests shouldn't affect building a bundle, right?

@brianlukoff
Copy link
Contributor

Thanks @dr-dimitru -- I'll take a look at switching to ostrio:flow-router-extra, although I'll still probably need to keep a fork of minifier-js around that disables the evaluate switch in terser, to avoid the risk of breaking other code.

@filipenevola
Copy link
Collaborator

@dr-dimitru yes, tests are not important for this matter.

@brianlukoff I don't think you should be afraid of this option in Terser. This is the first problem like this in a long time using it. We could isolate this in a test case inside flow router to make sure this doesn't happen in future versions.

Could you double check if @dr-dimitru fork doesn't manifest this problem? It would be really helpful.

@brianlukoff
Copy link
Contributor

@filipenevola @dr-dimitru I have confirmed that ostrio:flow-router-extra works fine with the default evaluate option in terser -- thanks! (I won't belabor the point, but to me the fact that we had to check to be sure just reinforces my earlier point -- we should be completely confident that Meteor's minification process will not change the semantics of any library!)

@filipenevola
Copy link
Collaborator

We are deprecating kadira:flow-router in favor of ostrio:flow-router-extra.

@dthwaite
Copy link

dthwaite commented Jan 19, 2022

Hi, Just to add my thoughts on this (having independently discovered and reported the issue in the Meteor forums.)

As @brianlukoff says, I do have serious concerns that the minifier incorrectly interpretes the code. Even if the code uses out-dated paradigms, it is not actually illegal/incorrect code. My code and that of many libraries I use still has variables declared with var. Surely the minifier should respect this syntactically correct code? It makes me wince to think of what other pieces of (old) code are being misinterpreted by the minifier.

Can we not hunt down the bug in minifier that is causing it to mis-interprete code (instead of changing a behaviour flag)?

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants