Disable workers with string literals and improve diagnostics #6536
Conversation
|
This pull request has been linked to and will mark 1 task as "Done" when merged:
|
| @@ -63,7 +65,7 @@ export type Diagnostic = {| | |||
| language?: string, | |||
|
|
|||
| /** A code frame points to a certain location(s) in the file this diagnostic is linked to (optional) */ | |||
| codeFrame?: ?DiagnosticCodeFrame, | |||
| codeFrame?: ?DiagnosticCodeFrame | ?Array<DiagnosticCodeFrame>, | |||
There was a problem hiding this comment.
We could possibly make this always an array if we wanted. I just didn't want to have to change all the tests. Any opinions?
There was a problem hiding this comment.
I think BuildFailureEvent originally also contained either a diagnostic or an array of them, and it was rather cumbersome to use because you always had to somehow normalize them into an array anyway yourself.
There was a problem hiding this comment.
Okay, I've refactored it to rename codeFrame to codeFrames and make it always an array. Also moved language and filePath into code frames rather than on the top-level diagnostic object.
Benchmark ResultsKitchen Sink 🚨
Timings
Cold BundlesNo bundles found, this is probably a failed build... Cached BundlesNo bundles found, this is probably a failed build... React HackerNews ✅
Timings
Cold BundlesNo bundle changes detected. Cached Bundles
AtlasKit Editor 🚨
Timings
Cold BundlesNo bundles found, this is probably a failed build... Cached BundlesNo bundles found, this is probably a failed build... Three.js ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. |
|
Turns out
|
|
Yeah I wondered the same. It just seemed weird to me to have filePath at the top-level if each code frame can refer to a different file. I think it's still possible to have a code frame with only a file path though, and no code highlights. Not really sure when that would happen though. |
* v2: (34 commits) Wrap assets recursively when any incoming dependency is wrapped (#6572) Improvements for library targets (#6570) Diagnostic for undeclared external dependencies in library builds (#6564) More bugs (#6567) Don't require `url:` for image transformer (#6565) Remove 'Name already registered with serializer' error (#6566) Fix live bindings and `this` of external CommonJS modules (#6548) JS runtime improvements (#6531) Make sure the absolute path isn't contained in the cache (#5900) Adds '@parcel/diagnostic' to dependencies (#6563) Disable workers with string literals and improve diagnostics (#6536) Bug fixes (#6541) Don't attempt to resolve URLs starting with '#' (#6504) Correctly set worker's output format if not support by environment (#6534) Babel: Recognize peerDependencies in isJSX (#6497) fix setHeaders ordering on dev server (#6500) Graph: Remove Node interface (#6530) Fix TS build script for old Node versions (#6526) Improve library targets (#6517) Fix TypeScript and other sourcemaps by always creating an initial sourcemap (#6472) ...
Closes T-1087
This disables workers of all types from being created with a string literal. It's now required to use
new Worker(new URL('filename.js', import.meta.url), {type: 'module'}). This is because strings passed to the worker constructor are supposed to be resolved relative to the page URL, not to the current file. Using theURLconstructor solves this issue so that Parcel is more web compatible. This is also how other bundlers like webpack now work.This also makes some formatting improvements to diagnostics, especially when multiple diagnostics are displayed at once. The code frame and hints are now indented underneath each error message so it's clear what it is associated with.
In addition, it's now possible to have multiple code frames pointing to different files within the same diagnostic. This way, errors can show context for the error in another file without creating a separate diagnostic which looks like a different error.
There's also some other small diagnostic improvements in this PR which you can see in the code.