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

[forked react-scripts] Overlay content contains unwanted chars #3831

Closed
thomasbertet opened this issue Jan 17, 2018 · 12 comments
Closed

[forked react-scripts] Overlay content contains unwanted chars #3831

thomasbertet opened this issue Jan 17, 2018 · 12 comments

Comments

@thomasbertet
Copy link

thomasbertet commented Jan 17, 2018

Hi there,

Is this a bug report?

No

❤️

Really love this project & what this community does, so first thanks for that ! :)

I'm not really reporting an issue, I'm more asking for an hint on where I'm missing something.

Context

I'm using a fork of react-scripts that lives inside a private repo.
I have kind of forked react-scripts, and customized it to my specific needs.
At this end, I manipulated a bit the files inside the config folder, and I'm reporting any new feature/bugfix on react-scripts to my project quite manually. This might be linked to my following issue 🙉
As a side note, I did fork like a year ago to be able to configure HMR, CSS Modules & SASS.

My issue:

  • The error message displayed in the overlay contains unwanted chars :
    selection_130

I'm wondering if any of you can point me to why this might happen. I'm guessing that maybe someone already ran into this issue.
I mean the overlay itself is displaying well, in the command line output, the results is great, but unfortunately not in the overlay's content :(

Related webpack config

in webpackDevServer.config.js I have the following

        before(app) {
            // This lets us open files from the runtime error overlay.
            app.use(errorOverlayMiddleware());
        },

in webpack.config.dev.js

    entry: {
        app: [
            // Include hot reload.
            require.resolve('react-hot-loader/patch'),
            // Include custom hotDevClient
            require.resolve('react-dev-utils/webpackHotDevClient'),
            // Finally, this is your app's code:
            paths.appIndexJs,
            // We include the app code last so that if there is a runtime error during
            // initialization, it doesn't blow up the WebpackDevServer client, and
            // changing JS code would still trigger a refresh.
        ],
    },

Big thanks to whoever can point me to the right direction !

@gaearon
Copy link
Contributor

gaearon commented Jan 17, 2018

This looks to me like you're not showing a build-time error early enough and so you end up showing a runtime error thrown in this case by webpack instead.

@gaearon
Copy link
Contributor

gaearon commented Jan 17, 2018

It might also be that you're showing both, but the runtime overlay z-index is higher than the build overlay z-index. Although in latest versions of react-dev-utils those overlays were unified so you should never see them both at the same time.

@thomasbertet
Copy link
Author

thomasbertet commented Jan 17, 2018

Thanks for your help !

I'm using the latest version of react-dev-utils (ie. ^5.0.0). It seems there are not two different error overlays at the same time.

The error is clearly thrown at build-time, I did a syntax mistake on purpose to try the error overlay.
It seems odd to me that the message in the command line is nice, but not the one inside the error overlay. Are they generated by two different mechanism ?

@gaearon
Copy link
Contributor

gaearon commented Jan 17, 2018

I think you want to debug why this doesn't get called.

@thomasbertet
Copy link
Author

Nice that is what I was looking for, a nice hint to start debugging. Thanks so much Dan 👍

@gaearon
Copy link
Contributor

gaearon commented Jan 17, 2018

You could create a new project and then look side by side at which events are received by the client.

@thomasbertet
Copy link
Author

Yep I have another project running a proper version of CRA, will do !

@thomasbertet
Copy link
Author

thomasbertet commented Jan 18, 2018

I found the issue, and now it works properly :)

What I did wrong when upgrading manually to latest react-dev-utils

In webpack.config.dev.js :
Before

entry: {
        app: [
                 [...]
                 'webpack-dev-server/client?http://localhost:' + CUSTOM_PORT,

After with the new webpackHotDevClient :

entry: {
        app: [
                 [...]
                require.resolve('react-dev-utils/webpackHotDevClient'),

As you can see -and it seems obvious now- I forgot to re-add the custom port I was using for the socket to connect.
Side-note: I'm using a proxy, thus, my app is not launched on port window.location.port, but on another one (CUSTOM_PORT). The socket needs to contact the app directly, not using my proxy server port.

webpackHotDevClient limitation

To fix my issue, I need to tell the dev client to create the socket on a custom port, for now it uses the window.location.port which is not the right one for me.

PR to add this feature

Will you accept a PR to add ability to set a custom port on the socket connection ? I was thinking either the same way as 'webpack-dev-server/client?http://localhost:' + CUSTOM_PORT or via an ENV variable ? If you find it's not required for the CRA ecosystem to add this complexity, that's fine by me, it might just serve my purpose, so I can understand.

Please tell me what you think :)

@gaearon
Copy link
Contributor

gaearon commented Jan 18, 2018

Can you search if this was proposed before? I don’t remember.

@thomasbertet
Copy link
Author

I found this revision that was just before you switched to custom dev client. You were doing require.resolve('webpack-dev-server/client') + '?/',

Then I found the first version of the webpackHotDevClient where you could not specify any custom location.

Not sure if this what you asked me to look for, anyway, it seems the custom dev client never had this ability.

@gaearon
Copy link
Contributor

gaearon commented Jan 18, 2018

I guess I'm asking if this is related to #1588 or not.

@thomasbertet
Copy link
Author

Oh I see ... Yes this is related. It fixes the main issue with having the correct socket port. Thanks for pointing it out, did not even think about checking if a PR was made about this... 👍

Another part I saw that is not compatible with using a proxy is the ability to open the editor when clicking on the overlay. We are doing a fetch without any ability to set the custom port/host. Also, when you will be able to do so, we will also need to set CORS headers. I will ask in this PR if this is worth adding as well.

Thanks anyway, I think we might close this issue.

@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.
Projects
None yet
Development

No branches or pull requests

2 participants