Skip to content

Various fixes#1409

Merged
LukeSheard merged 15 commits intomainfrom
feature/various-fixes
Feb 17, 2022
Merged

Various fixes#1409
LukeSheard merged 15 commits intomainfrom
feature/various-fixes

Conversation

@cristiano-belloni
Copy link
Contributor

@cristiano-belloni cristiano-belloni commented Feb 16, 2022

Various fixes, in preparation to #1382

Fix reload when serving an esbuild app / view

Various problems:

  1. App not reloading when code changes -> the isFirstCompilation condition prevented the apps from reloading on change, fixed order.
  2. Looking at the same app in >2 browser tabs caused an infinite loop of mutual reloading, because the ws server published to all apps when a new one connected. As a result, the "older" app would reload, find it's not their first build, re-connect. That would re-trigger a publish, cause the other one(s) to reload and so on. Fixed by publishing to all apps only on code change, but publishing only to the just-connected app on connection.
  3. We were watching the build correctly, but not using the new build result anywhere. As a result, the browser tab would reload, but not show new content. Fixed by setting the new result with the build callback.

Call formatError with the correct path

In esbuild-world, errors passed to formatError are relative to ModularRoot. If we pass them relative to the app path, instead, formatError tries to open a wrong path and kills the server with an exception. This fixes it.

Stop Esbuild dev server from hanging

Sometimes the esbuild dev server hangs indefinitely when it's killed, either from terminal or programatically during tests. To solve the first issue,it seems it's sufficient to terminate its clients one by one. To solve the second issue, I'm sending a kill -9 programatically.

Revert react-error-overlay to previous patch and inject process.env and process.platform

It seems that in the latest patch, react-error-overlay is not only trying to access process, but the code it loads in the iframe it creates does too. In both cases, if process is not defined, react-error-overlay throws and is not available at runtime. For the first issue, the solution is to just inject process in the runtime. For the second issue, it's not clear how CRA does that (maybe at build time with a shim? I don't think they use the packaged react-error-overlay). Since this issue has been probably introduced by an automatic update, the solution is to inject process to solve issue 1 and revert to the previous patch to solve issue 2.

@changeset-bot
Copy link

changeset-bot bot commented Feb 16, 2022

🦋 Changeset detected

Latest commit: ad57eb7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
modular-scripts Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coveralls
Copy link
Collaborator

coveralls commented Feb 16, 2022

Coverage Status

Coverage increased (+0.0009%) to 28.133% when pulling ad57eb7 on feature/various-fixes into f0e12ab on main.

@cristiano-belloni cristiano-belloni marked this pull request as ready for review February 16, 2022 17:10
@LukeSheard
Copy link
Contributor

@cristiano-belloni Looks good! Can we create a changeset for each of the fixes? Then we can merge this!

@cristiano-belloni
Copy link
Contributor Author

@cristiano-belloni Looks good! Can we create a changeset for each of the fixes? Then we can merge this!

Three changesets created, one per broad topic.

global: 'window',
},
banner: {
js: `window.process = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably needs a todo, web pack actually shims the process variable / other node.js concepts which is how they're defined. This is probably fine thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's the minimum viable injection to make react-error-overlay work.
There's a shim for it, yes (it even mocks complex stuff stuff like hrtime via the browser's timers) but our main problem with the newest version of reo is that the iframe tries to access process, and it's not trivial to inject it there.

publish();
});
const publishAll = () => {
// eslint-disable-next-line @typescript-eslint/no-misused-promises
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to resolve these lint warnings neatly?

Copy link
Contributor

@LukeSheard LukeSheard Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe

return Promise.all(server.clients.map(publishClient));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That and a bunch of void typings did the trick.

@LukeSheard LukeSheard merged commit f5c1cb8 into main Feb 17, 2022
@LukeSheard LukeSheard deleted the feature/various-fixes branch February 17, 2022 11:10
@github-actions github-actions bot mentioned this pull request Feb 17, 2022
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.

3 participants