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

Elm: typechecking newly added imported file doesn't trigger reload #2147

Closed
domenkozar opened this issue Oct 15, 2018 · 11 comments · Fixed by #2475
Closed

Elm: typechecking newly added imported file doesn't trigger reload #2147

domenkozar opened this issue Oct 15, 2018 · 11 comments · Fixed by #2475
Labels

Comments

@domenkozar
Copy link

domenkozar commented Oct 15, 2018

🐛 bug report

In file B, importing a new file A that doesn't initially typecheck doesn't trigger reloads on subsequent changes.

🎛 Configuration (.babelrc, package.json, cli command)

Barebones elm project.

🤔 Expected Behavior

When file A is saved, parcel reloads.

😯 Current Behavior

Nothing happens when saving file A, one needs to save file B.

💁 Possible Solution

🌍 Your Environment

Software Version(s)
Parcel 1.10.3
Operating System Linux
@domenkozar domenkozar changed the title Elm: importing Elm: typechecking newly added imported file doesn't trigger reload Oct 15, 2018
@domenkozar
Copy link
Author

cc @benthepoet

@domenkozar
Copy link
Author

Could be an upstream bug in https://github.com/NoRedInk/find-elm-dependencies#usage, need to test that next time it happens.

@domenkozar
Copy link
Author

Got this again today. findAllDependencies('./src/Main.elm').then(function(deps) { console.log(deps); }); did return the offending file. Restarting parcel fixes the issue.

@DeMoorJasper any tips how to debug how parcel picks up file changes?

@domenkozar
Copy link
Author

The problem might be that there is an error:

    at ChildProcess.<anonymous> (/home/ielectric/dev/cachix/cachix-server/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)

@MattCheely
Copy link

I did some research on this and it seems the issue is that when the compile process throws an error, all processing of the asset halts, and any collected information such as dependencies is thrown away. At a high level, this happens in Bundler.js on line 556, and dependencies aren't watched until line 578.

There are a few ways to potentially fix this, but I think the ideal situation would be to let Asset.generate return something that indicates that the asset is in error, while still retaining other data. Maybe something as simple as {error: '...'}. One nice thing about that would be that it would make errors more of a first-class part of the pipeline and open the door for things like customizing the error display in nice ways. That could be pretty interesting with tools like Elm and Rust that produce really nice error information.

I'd be happy to try tackling this problem, but I want to make sure I'm doing work that makes sense in the context of the larger project. @DeMoorJasper or @devongovett - What's the best way to approach this? I can just start slinging code and open a PR, but if there's a particular approach you think would be best, I'm happy to go that route.

MattCheely added a commit to MattCheely/parcel that referenced this issue Dec 2, 2018
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.
domenkozar added a commit to domenkozar/parcel that referenced this issue Dec 26, 2018
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.
MattCheely added a commit to MattCheely/parcel that referenced this issue Dec 26, 2018
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.
MattCheely added a commit to MattCheely/parcel that referenced this issue Dec 27, 2018
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.
MattCheely added a commit to MattCheely/parcel that referenced this issue Dec 28, 2018
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.
DeMoorJasper pushed a commit that referenced this issue Jan 3, 2019
)

* 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.

* 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
@sklirg
Copy link

sklirg commented Jan 22, 2019

Hi there. Sorry for commenting on this closed issue, but I'm not sure if I should raise a new issue since this feature isn't released yet.

I still experience this bug even after trying the fork mentioned in #2475 (@domenkozar: Did you do anything specific to make it work? Could you look at the example repo and see if you're able to reproduce it too?).

Steps to reproduce:

  1. Edit text in Feat.elm, which is imported by Main.elm
  2. Save Feat.elm
  3. Expect hot reload
  4. No hot reload
  5. Save Main.elm
  6. Hot reload happens

See this example repo and this demo video.

Any suggestions on what I can do to solve this problem?

Thanks a bunch!

@domenkozar
Copy link
Author

How did you install the fork? To valifate you're using correct ckde, you should not see duplicate elm errors in the logs.

@sklirg
Copy link

sklirg commented Jan 23, 2019 via email

@domenkozar
Copy link
Author

I can reproduce on your repository, but for me it works well with the same package.json pins. Not sure yet what's going on.

@domenkozar
Copy link
Author

Ah you're missing module Main exposing (main) on top of your Main.elm.

MattCheely pushed a commit to MattCheely/elm-app-gen that referenced this issue Jan 30, 2019
This updates projects to use a fork of parcel that includes a fix
for parcel-bundler/parcel#2147  once
that fix is relased in the official parcel package, we should
switch back, and open a PR against parcel-bundler-fork to notify
users to update their dependencies.
MattCheely pushed a commit to MattCheely/elm-app-gen that referenced this issue Jan 30, 2019
This updates projects to use a fork of parcel that includes a fix
for parcel-bundler/parcel#2147  once
that fix is relased in the official parcel package, we should
switch back, and open a PR against parcel-bundler-fork to notify
users to update their dependencies.
@domenkozar
Copy link
Author

This is now fixed in 1.12 (although changelog doesn't mention that).

wbinnssmith pushed a commit that referenced this issue Apr 11, 2019
)

* 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.

* 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
wbinnssmith pushed a commit that referenced this issue Apr 12, 2019
)

* 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.

* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants
@domenkozar @MattCheely @DeMoorJasper @sklirg and others