madsflensted / elm-brunch

Brunch plugin to compile Elm code
MIT License
74 stars 31 forks source link

New way to create Brunch plugins #11

Open urfolomeus opened 8 years ago

urfolomeus commented 8 years ago

It looks like there is a new template for creating Brunch plugins.

I discovered this when someone raised an issue on my SeatSaver repo. We were seeing strange behaviour when the elm file was in the watchlist, but everything worked fine when it wasn't. After a bit of poking around (and discussing with the fine folks on the Elixir Slack #phoenix channel) I realised that it was the latest release of Brunch (2.2.0) that had caused the issue and that the method for creating plugins had changed. I hacked out a quick spike to test and it seems to work fine on my environment (albeit much more simple and hardwired to my use case).

I'll hopefully have time to hack out a proper solution tomorrow evening if you like?

tobyhede commented 8 years ago

semver, how does it even!!?

polymetis commented 8 years ago

I ran into this on the weekend. Slow to the draw I guess. :P @madscoaducom This is what I was talking about on twitter but I wanted to make a proper bug report. If you need any help feel free to ping me but @urfolomeus 's spike seem to work here as well.

tobyhede commented 8 years ago

I am a bit rusty on the node ecosystem but happy to help test a PR.

madsflensted commented 8 years ago

@urfolomeus Thanks for taking the time to investigate the details. For now I have added a warning to the README.

I wonder how we should handle the users that stay with brunch < 2.2.0 ...hurray for backwards incompatible changes.

I will try to look at this tonight.

urfolomeus commented 8 years ago

@madsflensted I've had a first pass at getting the existing test suite (well a slightly modified one) to pass. You can see it on my spike.

Will give it a go in my Phoenix/Elm app and see how it goes.

Some caveats:

1) This is very much me just poking about to see how far I can go. Happy to drop that repo entirely and bring the results in here if they're useful. 2) This is just a first pass, I've not tried it in the Phoenix/Elm app yet to see if it works (will do that now). Also I may have missed some existing functionality.

Hope this helps :)

urfolomeus commented 8 years ago

Um well, it would seem I have more work to do 😄

(not quite working yet)

madsflensted commented 8 years ago

I have been doing some debugging in brunch and the plugin. I have found that brunch supports the old plugin model still, but it seems that somewhere (I have not found it yet) - the fact that elm-brunch returns data = null to the callback, directs the brunch code to try and access '.path' of undefined.

If I make a slight change to this line and call the callback with "" instead of null on success - the error goes away.

return callback(error, error ? stderr : "");

It also looks like all the files are generated properly, but I do not feel comfortable about this.

@polymetis @urfolomeus and @tobyhede could you try this hack in your local node_modules/elm-brunch/index.js and see if that fixes it for your? (when you run with brunch 2.2.1)

urfolomeus commented 8 years ago

Works for me 😊 Nice find and much simpler than my approach.

madsflensted commented 8 years ago

Ok - I have published a 0.4.2 with this workaround. I suspect that the side effect is that the rest of the pipeline is run with a empty source string, but this is probably a minor issue for Elm projects.

Going forward I think re-writing to the new plugin format, is the way to go.

tobyhede commented 8 years ago

Works for me too

On 27 January 2016 at 18:04, Mads Flensted-Urech notifications@github.com wrote:

Ok - I have published a 0.4.2 with this workaround. I suspect that the side effect is that the rest of the pipeline is run with a empty source string, but this is probably a minor issue for Elm projects.

Going forward I think re-writing to the new plugin format, is the way to go.

— Reply to this email directly or view it on GitHub https://github.com/madsflensted/elm-brunch/issues/11#issuecomment-175453574 .