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

Fix webpack config errors #339

Merged
merged 4 commits into from
Jul 4, 2022
Merged

Fix webpack config errors #339

merged 4 commits into from
Jul 4, 2022

Conversation

will-jac
Copy link
Collaborator

@will-jac will-jac commented Apr 2, 2022

This solves issue #165 by changing the devtool used by webpack to the correct one.

This also solves issue #164 by properly bundling the monaco webworker via changes to the webpack config

@will-jac will-jac changed the title fixed fetch errors when running in dev mode Fix errors in the console Apr 2, 2022
@will-jac will-jac changed the title Fix errors in the console Fix webpack config errors Apr 2, 2022
@navzam
Copy link
Member

navzam commented Apr 15, 2022

@will-jac do you know what it is about eval source maps that causes #165 ?

@will-jac
Copy link
Collaborator Author

Not entirely sure. The change was based on this issue I found, although it may be outdated. This article suggests minification may be the issue, but I'm not sure.

Given that the documentation does not suggest this config, I would hold off from merging this PR. I can do some additional investigation on other options.

@navzam
Copy link
Member

navzam commented Apr 15, 2022

Not entirely sure. The change was based on this issue I found, although it may be outdated. This article suggests minification may be the issue, but I'm not sure.

Given that the documentation does not suggest this config, I would hold off from merging this PR. I can do some additional investigation on other options.

Gotcha. I don't mind changing to inline but am curious what the underlying problem is

@navzam
Copy link
Member

navzam commented Apr 15, 2022

Also the Monaco issue you're fixing is #163 right? Though it seems #164 also gets fixed by the devtool change

@will-jac
Copy link
Collaborator Author

Also the Monaco issue you're fixing is #163 right? Though it seems #164 also gets fixed by the devtool change

This should fix both, since I think they're manifestations of the same error

…e- does not seem to be recommended by the documentation
@will-jac
Copy link
Collaborator Author

Gotcha. I don't mind changing to inline but am curious what the underlying problem is

There is some discussion here - with the eval flag set, it (DevTools) can't find the sourcemap. So the solution is to not use it or use inlined sourcemaps. This may be why the FetchAPI is failing to load the file.

Let's do some testing!

Working:

  • [cheap-][module-]source-map

Not Working:

  • (none)
  • eval-*

It's odd that not setting the devtool leads to an error - the errors are also different, although the Fetch error is the same:

DevTools failed to load source map: Could not load content for webpack:///node_modules/styletron-react/dist/browser.es5.es.js.map: Fetch through target failed: Unsupported URL scheme; Fallback: HTTP error: status code 404, net::ERR_UNKNOWN_URL_SCHEME

This leads me to believe that there could be an error in prod, although I don't see one in the web console.

After all of this, I was thinking that the error could be with our express app not sending the sourcemaps, so I tried running webpack-dev-server, but had the same errors. For now, I think using source-map (which is suggested by the documentation as a config) is probably our best bet.

@navzam
Copy link
Member

navzam commented Jun 13, 2022

Sorry @will-jac forgot about this one. So it sounds like any eval- ones are the problem. My only concern about source-map is how slow builds/rebuilds are, since it makes development harder. You're saying cheap-module-source-map works right? That seems to rebuild reasonably fast

@will-jac
Copy link
Collaborator Author

Yes, that's what I would suggest using. You should double-check that it works after all the recent changes to the simulator.

@navzam
Copy link
Member

navzam commented Jun 16, 2022

@will-jac sounds good, I'll change that, verify it, and merge this if you're good with that

@navzam navzam merged commit af2992f into kipr:master Jul 4, 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.

2 participants