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

Update for Elm 0.19 #142

Merged
merged 15 commits into from
Aug 29, 2018
Merged

Update for Elm 0.19 #142

merged 15 commits into from
Aug 29, 2018

Conversation

xtian
Copy link
Contributor

@xtian xtian commented May 10, 2018

Fixes #148

@@ -38,9 +37,7 @@ var getOptions = function() {
? this.options.elm || {}
: this.query.elm || {};
var loaderOptions = loaderUtils.getOptions(this) || {};
return Object.assign({
emitWarning: this.emitWarning
Copy link
Contributor Author

@xtian xtian May 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@klazuka
Copy link
Contributor

klazuka commented Aug 6, 2018

What’s the status on this PR? Is it just on hold until support for 0.18 can be dropped?

I ask because I’m working on 0.19-compatible hot reloading based on fluxxu/elm-hot-loader, and one of the last remaining things to do is ensure that it can plug into webpack’s HMR API. If this PR is ready from a technical perspective, then I will go ahead and develop against this branch/fork.

@xtian
Copy link
Contributor Author

xtian commented Aug 8, 2018

@klazuka As far as I have tested it is ready from a technical perspective.

@klazuka
Copy link
Contributor

klazuka commented Aug 10, 2018

@xtian I integrated your branch today. The code itself worked fine (thank you!), but some of the docs/examples are out-of-date. Here are the list of changes that I had to make to get things working:

webpack options

  • pathToMake has become pathToElm
  • warn is no longer support by the Elm compiler nor node-elm-compiler

various things in the example-wp4 directory

(presumably similar problems exist in wp2)

  • replace elm-package.json with elm.json
  • index.js does not initialize the main Elm module correctly (it uses embed, but it should use init)
  • index.js should import the Elm app as const { Elm } = require('./Main.elm');, otherwise it gets double-wrapped and you need to do Elm.Elm.Main.init

various other things

  • package.json at the root installs Elm 0.18 locally (no good alternative for this now, but noting it so that it gets done when 0.19 is published publicly)
  • test/loader.js still references elm-package.json on line 11

@xtian
Copy link
Contributor Author

xtian commented Aug 11, 2018

@klazuka Ah yes, sorry I should have noted that the changes on the app side can be viewed here: halfzebra/create-elm-app#256

Glad it worked for you! :)

@klazuka
Copy link
Contributor

klazuka commented Aug 11, 2018

No problem. But that PR only addresses the app-side in create-elm-app. There's a lot of docs/readmes/examples in this repo that also need to be updated. Probably should be done in a separate PR. Just pointing it out here so that it doesn't slip through the cracks.

@xtian
Copy link
Contributor Author

xtian commented Aug 11, 2018

@klazuka Yep, I am waiting until I get some response to this PR before diving into updating the documentation.

@xtian
Copy link
Contributor Author

xtian commented Aug 21, 2018

ping @rtfeldman @eeue56

This is ready for review

@klazuka
Copy link
Contributor

klazuka commented Aug 21, 2018

@xtian good news, the npm installer for 0.19 was released this morning. Now you can do npm install elm@rc1 to get 19. Could you please update the Elm dependency in package.json so that it no longer refers to 0.18? My hot-loader project depends on your branch, and the 0.18 dependency is causing a conflict. Thank you!

@xtian
Copy link
Contributor Author

xtian commented Aug 21, 2018

@klazuka Updated :)

@Punie
Copy link

Punie commented Aug 21, 2018

@xtian I think an option for optimize: true should be added to provide the new --optimize flag from elm make.
Otherwise, I just tested your PR on my own project alongside @klazuka 's elm-hot and it works like a charm, thanks 👍

@hecrj
Copy link

hecrj commented Aug 22, 2018

I tried this branch. It seems that live reloading doesn't work when saving imported modules.

It looks like find-elm-dependencies needs to be updated so it looks for elm.json instead of elm-package.json. Then node-elm-compiler, which uses it, will need to update the dependency (and probably support the new --optimize flag, as @Punie said).

I have made a dirtyfix for myself in the meantime here: https://github.com/hecrj/elm-webpack-loader/tree/0.19

@daegren daegren mentioned this pull request Aug 22, 2018
@xtian
Copy link
Contributor Author

xtian commented Aug 22, 2018

@hecrj Awesome, thanks for posting a fix!

I don't have the bandwidth to tackle PRs for those projects right now so if someone else wanted to do that it would be very cool.

@rlopzc rlopzc mentioned this pull request Aug 23, 2018
9 tasks
@eeue56
Copy link
Contributor

eeue56 commented Aug 23, 2018

PRs for both of those repos would be very welcome!

@Punie
Copy link

Punie commented Aug 23, 2018

If @hecrj is willing to move his fixes to PRs on those repos. If he doesn't have the time, I can try to takle this when I get home in the evening.

[Edit]: I made those PRs (as far as I could tell, the changes needed were pretty minimal, but I might have missed something. I saw that progress towards 0.19 was already made on node-elm-compiler which lacked only the --optimize flag and the upgraded version of find-elm-dependencies)

@razzeee
Copy link

razzeee commented Aug 26, 2018

Can we maybe not hard remove pathToMake and warn and instead show an error pointing to the reason or a deprecation notice?

@eeue56
Copy link
Contributor

eeue56 commented Aug 28, 2018

@razzeee This release will break anyway, so I'm not sure if we can do that. Better to have a hard error and send people here to pin to the 0.18 version.

Just waiting on one last PR to be merged, then we're good to go with this release

@razzeee
Copy link

razzeee commented Aug 28, 2018

Well the hard error you get at the moment is quiet unhelpful. That's the whole reason I reached out. I think we can do much better, otherwise you will have people asking here regularly.

@rtfeldman
Copy link
Member

I like the idea of keeping them for this release, and having them immediately bail out with a helpful error if you use them. 👍

@rtfeldman
Copy link
Member

rtfeldman commented Aug 28, 2018

We ended up with some interesting experimental Uglify results in a discussion with @Punie on Slack that I thought I'd share here: ("two step version" refers to running Uglify twice, first with --compress and then again with --mangle - see the elm-spa-example README for why this helps)

screen shot 2018-08-28 at 8 35 47 am

So the overall best (for elm-spa-example at least) still seems to be running uglify-js twice (once with --compress and then again, separately, with --mangle) with passes=2. Second best is running uglify-es once with passes=3. Everything else did the same or worse than those two configurations.

Assuming it's too much of a pain to get Webpack to run Uglify twice, it seems like the best Uglify config for Webpack might be:

  • Use uglify-es over uglify-js
  • passes=3
  • both --compress and --mangle enabled at once

@Punie
Copy link

Punie commented Aug 28, 2018

Just to add to @rtfeldman comment: it seems that uglify-es is the default minifier used by the UglifyJS Webpack Plugin for Webpack 4 (see webpack-contrib/uglifyjs-webpack-plugin) so the following configuration should suffice :

new UglifyJsPlugin({
  uglifyOptions: {
    compress: {
      pure_funcs: ['F2','F3','F4','F5','F6','F7','F8','F9','A2','A3','A4','A5','A6','A7','A8','A9'],
      pure_getters: true,
      keep_fargs: false,
      unsafe_comps: true,
      unsafe: true,
      passes: 3
    }
  }
})

I don't know about webpack 2 and 3 though.

@eeue56
Copy link
Contributor

eeue56 commented Aug 28, 2018

@rtfeldman / @Punie can you share those somewhere else rather in this PR? 🙏 Maybe a separate PR with uglify tips?

@sentience sentience self-assigned this Aug 29, 2018
@eeue56 eeue56 merged commit dd74085 into elm-community:master Aug 29, 2018
@eeue56
Copy link
Contributor

eeue56 commented Aug 29, 2018

Released as 5.0.0. Thanks!

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.

9 participants