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

Defer throwing asset errors until after dependencies are handled. #2475

Merged
merged 9 commits into from Jan 3, 2019

Conversation

Projects
None yet
3 participants
@MattCheely
Copy link
Contributor

MattCheely commented Dec 28, 2018

↪️ Pull Request

Previously submitted in #2468 and #2344 (sorry about the confusion)

This allows compiled langauges like Elm that handle their own dependency
compilation to set up watchers for dependencies so that hot-reload
continues to work due to errors in new depdenencies. Fixes #2147.

There are also a few tweaks to ElmAsset to make sure compilation errors
are thrown after dependency collection

💻 Examples

Main.elm:

module Main exposing (main)

import Html exposing (text)


main =
    text "Hello World"

Broken.elm:

module Broken exposing (anError)

{- This module causes a compiler error -}


anError : String
anError =
    2

🚨 Test instructions

Using the files above, start the parcel server, and navigate to localhost:1234.

Add a line in Main.elm to import Broken.elm:

import Broken

Notice that the build fails

Update Broken.elm by changing the value 2 to "foo".

In current parcel: no rebuild is triggered
In this branch: the error is resolved

✔️ PR Todo

I will try to create a test for this in the hmr integration tests, now that I am more familiar with the tests.

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

Matthew Cheely added some commits Dec 1, 2018

Matthew Cheely
Defer throwing asset errors until after dependencies are handled.
This allows compiled langauges like Elm that handle their own dependency
compilation to set up watchers for dependencies so that hot-reload
continues to work due to errors in new depdenencies. Fixes #2147.
Matthew Cheely
Fix Elm error generation.
I'm not entirely sure why this fix is necessary, but without it a
compile error during a rebuild results in Parcel printing "Unknown
error" instead of anything useful. Since we are intecepting the error
though, we can also remove redundant stack information.
generateErrorMessage(err) {
// For some reason, if not converted to a plain object,
// the error message is lost somewhere between Pipeline.js
// and Bundler.js.

This comment has been minimized.

@MattCheely

MattCheely Dec 28, 2018

Contributor

This fixes an issue for Elm, but I probably also need to do something like this in Pipeline.js for other asset types that throw Errors. I'm not certain why this is necessary, but I suspect that there's an object copy somewhere in WorkerFarm that causes data to be lost if Pipeline.process returns an obejct containing an Error

This comment has been minimized.

@MattCheely

MattCheely Dec 28, 2018

Contributor

This is fixed.

Matthew Cheely
Transform all errors in Pipeline.process.
This prevents error data from being lost when Pipeline is run via
`WorkerFarm`.
@domenkozar

This comment has been minimized.

Copy link

domenkozar commented Dec 28, 2018

@MattCheely tests still seem to fail (although they were fixed yesterday).

@MattCheely

This comment has been minimized.

Copy link
Contributor

MattCheely commented Dec 28, 2018

@domenkozar Tests have been working for me locally for the most part, but I have noticed occasional timeouts on the hmr tests (mine and others), which seems to be part of the problem here. I don't have much insight into what's causing it, but there are a couple of things I can do to try simplifying the tests.

@domenkozar

This comment has been minimized.

Copy link

domenkozar commented Dec 28, 2018

@DeMoorJasper is that a known issue?

@DeMoorJasper

This comment has been minimized.

Copy link
Member

DeMoorJasper commented Dec 28, 2018

@domenkozar it only appears to be a consistent issue with this PR. But it’s a known issue that the tests are no longer stable.

Sent with GitHawk

@MattCheely

This comment has been minimized.

Copy link
Contributor

MattCheely commented Dec 28, 2018

@DeMoorJasper I tidied up my hmr tests a bit and the last set of failures in the test run don't seem to be related to my changes. Let me know if there's something I should do to resolve that.

FWIW, one thing I have noticed is that I see hmr tests timeout more frequently when running them in isolation via: yarn run mocha --grep hmr. Usually it's the electron test that times out. When I run the full test suite, I don't think I've seen it fail, so there may be some cross-test interaction or subtle timing issues at play.

@DeMoorJasper

This comment has been minimized.

Copy link
Member

DeMoorJasper commented Dec 28, 2018

@MattCheely Looks like you fixed it if I look at the CI results, not sure why the babel autoinstall is failing but that's probably another issue unrelated to this PR

@domenkozar

This comment has been minimized.

Copy link

domenkozar commented Dec 31, 2018

I'd love to test this in my project, but couldn't find a way or documentation how to install parcel from a local folder.

@MattCheely

This comment has been minimized.

Copy link
Contributor

MattCheely commented Dec 31, 2018

@domenkozar to test in your project locally, run yarn build at the top of the parcel repo, then in packages/core/parcel-bundler run yarn link, which will make that folder available for linking in other projects. Then in the project you want to test with, run yarn link parcel-bundler, which will create a symlink from node_modules/parcel-bundler in that project to your local checkout.

Edit: Or are you running globally installed parcel from the command line, rather than as a local project dependency?

@domenkozar

This comment has been minimized.

Copy link

domenkozar commented Dec 31, 2018

Thanks @MattCheely - it does fix the issue. I wonder if error messages are fixed though, I still see two of them:

Server running at http://localhost:12345 
🚨  /home/ielectric/dev/monorepo/frontend/src/Main.elm: Compilation failed
[================                                  ] - 1 / 3-- PARSE ERROR ----------------------------------------------------- src/Foo.elm

Something went wrong while parsing bar's definition.

4| bar = 
5| 
   ^
I was expecting to see the rest of bar's definition. Maybe you forgot some code?
Or maybe the body of `bar` needs to be indented?
Detected errors in 1 module.                                         

[================                                  ] - 1 / 3-- PARSE ERROR ----------------------------------------------------- src/Foo.elm

Something went wrong while parsing bar's definition.

4| bar =
5|
   ^
I was expecting to see the rest of bar's definition. Maybe you forgot some code?
Or maybe the body of `bar` needs to be indented?
Detected errors in 1 module.

    at ChildProcess.<anonymous> (/home/ielectric/dev/monorepo/frontend/node_modules/node-elm-compiler/index.js:149:27)
    at emitTwo (events.js:126:13)
    at ChildProcess.emit (events.js:214:7)
    at maybeClose (internal/child_process.js:925:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:209:5)
@domenkozar

This comment has been minimized.

Copy link

domenkozar commented Dec 31, 2018

My thinking is that due to https://github.com/rtfeldman/node-elm-compiler/blob/master/src/index.ts#L126 we can't get elm not to report the error, so we could only tell parcel to skip it.

@domenkozar

This comment has been minimized.

Copy link

domenkozar commented Dec 31, 2018

@MattCheely I think there was a typo, now it works correctly with:

diff --git a/packages/core/parcel-bundler/src/assets/ElmAsset.js b/packages/core/parcel-bundler/src/assets/ElmAsset.js
index 5a0f4cc..5f81ebc 100644
--- a/packages/core/parcel-bundler/src/assets/ElmAsset.js
+++ b/packages/core/parcel-bundler/src/assets/ElmAsset.js
@@ -135,7 +135,7 @@ class ElmAsset extends Asset {
   generateErrorMessage(err) {
     // The generated stack is not useful, but other code may
     // expect it and try to print it, so make it an empty string.
-    err.stack == '';
+    err.stack = '';
     return err;
   }
 }

@domenkozar

This comment has been minimized.

Copy link

domenkozar commented Dec 31, 2018

Happy new year 🎉 thanks for all of the work with above patch it works great!

DeMoorJasper added some commits Dec 31, 2018

@MattCheely

This comment has been minimized.

Copy link
Contributor

MattCheely commented Dec 31, 2018

@domenkozar I will see if I can get that working again. It should be possible, at least for the parcel browser overlay. I probably accidentally reverted my previous fix when I was changing the way errors are handled in Pipeline. If I can get it fixed, I'll add a test for it so It doesn't accidentally regress again.

@MattCheely

This comment has been minimized.

Copy link
Contributor

MattCheely commented on e1f027f Dec 31, 2018

Doh! 🤦‍♂️

@domenkozar

This comment has been minimized.

Copy link

domenkozar commented Jan 1, 2019

Do tests run in parallel? There are some elm corruption errors that are known issue if you mess around with cache or run things in parallel. elm/compiler#1845

Maybe doing rm -rf elm-stuff before each test could help (or separate working directories).

@domenkozar

This comment has been minimized.

Copy link

domenkozar commented Jan 1, 2019

Ah actually that's probably an old annotation. https://dev.azure.com/devongovett/devongovett/_build/results?buildId=627 shows only the known transient error.

@DeMoorJasper can we merge this?

@DeMoorJasper

This comment has been minimized.

Copy link
Member

DeMoorJasper commented Jan 2, 2019

@domenkozar unfortunately only Devon can merge PRs with failing tests.
Not sure if Azure DevOps can be configured to not block merging if tests fail as long as they're not stable

@domenkozar

This comment has been minimized.

Copy link

domenkozar commented Jan 2, 2019

seems to pass now :)

@DeMoorJasper DeMoorJasper merged commit c3d99de into parcel-bundler:master Jan 3, 2019

@domenkozar

This comment has been minimized.

Copy link

domenkozar commented Jan 3, 2019

Thank you! Now we wait for the Parcel 1.11.1 that will make Elm development with Parcel a breeze :)

maumercado added a commit to maumercado/parcel that referenced this pull request Jan 4, 2019

Defer throwing asset errors until after dependencies are handled. (pa…
…rcel-bundler#2475)

* Defer throwing asset errors until after dependencies are handled.

This allows compiled langauges like Elm that handle their own dependency
compilation to set up watchers for dependencies so that hot-reload
continues to work due to errors in new depdenencies. Fixes parcel-bundler#2147.

* Fix Elm error generation.

I'm not entirely sure why this fix is necessary, but without it a
compile error during a rebuild results in Parcel printing "Unknown
error" instead of anything useful. Since we are intecepting the error
though, we can also remove redundant stack information.

* Add test for tracking dependencies on error

* revert hmr-runtime changes

* Transform all errors in Pipeline.process.

This prevents error data from being lost when Pipeline is run via
`WorkerFarm`.

* Separate error-depenency test input from basic Elm tests.

* Update ElmAsset.js

maumercado added a commit to maumercado/parcel that referenced this pull request Jan 4, 2019

Defer throwing asset errors until after dependencies are handled. (pa…
…rcel-bundler#2475)

* Defer throwing asset errors until after dependencies are handled.

This allows compiled langauges like Elm that handle their own dependency
compilation to set up watchers for dependencies so that hot-reload
continues to work due to errors in new depdenencies. Fixes parcel-bundler#2147.

* Fix Elm error generation.

I'm not entirely sure why this fix is necessary, but without it a
compile error during a rebuild results in Parcel printing "Unknown
error" instead of anything useful. Since we are intecepting the error
though, we can also remove redundant stack information.

* Add test for tracking dependencies on error

* revert hmr-runtime changes

* Transform all errors in Pipeline.process.

This prevents error data from being lost when Pipeline is run via
`WorkerFarm`.

* Separate error-depenency test input from basic Elm tests.

* Update ElmAsset.js
@domenkozar

This comment has been minimized.

Copy link

domenkozar commented Jan 9, 2019

@DeMoorJasper do you have an idea if Parcel bugfix release will happen anytime soon? So I either look into setting up using Parcel master or just wait for bugfix release. Thanks!

@domenkozar

This comment has been minimized.

Copy link

domenkozar commented Jan 10, 2019

I uploaded parcel-bundler-fork to npm based on the current master to unblock my workflow. I plan to delete it as soon as 1.11.1 is out - thank you all again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment