Add support for Elm assets #1968

benthepoet · 2018-08-31T14:03:37Z

This PR adds support for assets written with Elm 0.19. Some notes are as follows.

Elm assets require a full page reload when using HMR (updates can't be applied reliably). For this I added a forceReload flag to the Asset class so that any asset that requires a reload can be indicated as such. Aside from Elm assets, HTML assets also have this flag turned on.
Minification is performed using a ~~two-pass~~ configuration as recommended by the official Elm docs (http://elm-lang.org/0.19.0/optimize)

kzc · 2018-08-31T15:28:27Z

Minification is performed using a two-pass configuration as recommended by the official Elm docs (http://elm-lang.org/0.19.0/optimize)

The two pass thing is due to a known unfixed regression in uglify-js in pure_funcs when the rename pass was introduced prior to the compress phase. Even so, mangle and compress can still work correctly with just a single uglify minify invocation by disabling this rename pass - see rtfeldman/elm-spa-example#54

This pure_funcs rename bug is not present in terser, as the rename pass is internally disabled - its flags are just no-ops. As result only a single minify invocation is needed.

benthepoet · 2018-08-31T15:41:22Z

Thanks @kzc, I wasn't aware of that. I've updated it so it's just a single operation now.

kzc · 2018-08-31T15:56:57Z

src/assets/ElmAsset.js

+          unsafe: true,
+          unsafe_comps: true
+        },
+        mangle: true


In the unlikely event that the Terser maintainer reintroduces the rename pass, please explicitly disable the option:

rename: false,

kzc · 2018-08-31T16:00:03Z

src/assets/ElmAsset.js

+      };
+
+      const {code} = minify(source, options);
+      return code;


The minify return error property should be examined. Terser will not throw upon minify error. Consult its API docs.

kzc · 2018-08-31T16:00:56Z

src/assets/ElmAsset.js

+      };
+
+      const {code} = minify(source, options);
+      return code;


The minify return error property should be examined. Terser will not throw upon minify error. Consult its API docs.

kzc · 2018-08-31T16:17:36Z

I noticed in the original docs for https://github.com/rtfeldman/elm-spa-example#step-2 that it specified the compress option passes: 2 as an increased number of compress passes could potentially produce smaller minified output. It's up to you.

benthepoet · 2018-08-31T16:41:25Z

@kzc I believe all the feedback should be accounted for now. I also set compress to perform 2 passes (probably helps to be consistent with the other example).

kzc · 2018-08-31T16:45:19Z

Cool. I've never used Elm before - looks interesting. I should check it out.

DeMoorJasper

Awesome, thanks for this, looks pretty much good to go.

Added some comments for minor improvements (the biggest one being config for cache invalidation)

DeMoorJasper · 2018-09-02T03:58:29Z

src/Asset.js

@@ -25,6 +25,7 @@ class Asset {
    this.options = options;
    this.encoding = 'utf8';
    this.type = path.extname(this.name).slice(1);
+    this.forceReload = false;


Not sure if this should be called forceReload, at first I thought this meant a parcel reload or something.

Perhaps hmrPageReload or something?

test/integration/elm/elm.json

DeMoorJasper · 2018-09-02T18:27:09Z

package.json

@@ -82,7 +82,9 @@
    "codecov": "^3.0.0",
    "coffeescript": "^2.0.3",
    "cross-env": "^5.1.1",
+    "elm": "^0.19.0-bugfix2",


Perhaps change this to ^0.19.0 not sure why they're appending -bugfix instead of just bumping the patch version

Yeah, I'm not sure why they decided to tag it like that. Makes sense to keep it as ^0.19.0.

DeMoorJasper · 2018-09-02T18:33:31Z

src/assets/ElmAsset.js

+      this.name
+    );
+    const dependencies = await findAllDependencies(this.name);
+    const packageFile = await findPackageFile(path.dirname(this.name));


Change this to:

await this.getConfig(['elm.json'], {load: false});

This automatically recursively searches for the file and adds it to the deps, without reading it.

This way you can remove your helper function, cleans it up a little

devongovett · 2018-09-22T19:36:41Z

src/assets/ElmAsset.js

+
+  async collectDependencies() {
+    const {findAllDependencies} = await localRequire(
+      'find-elm-dependencies',


I think we can avoid installing this package directly if we use elm.findAllDependencies from the node-elm-compiler package, which already depends on find-elm-dependencies. This way only one package will end up getting installed in a user's app.

devongovett · 2018-09-22T19:39:26Z

package.json

@@ -82,7 +82,9 @@
    "codecov": "^3.0.0",
    "coffeescript": "^2.0.3",
    "cross-env": "^5.1.1",
+    "elm": "^0.19.0",


What is this dependency used for? I don't see it used anywhere in the code.

This provides the actual elm binaries that node-elm-compiler uses to compile the source files. The package isn't a dependency of node-elm-compiler, so unfortunately we have to pull it in.

Ah I see. So since this is a dev dependency in parcel, so we might need to install it in a user's project as well, right?

Yes, that's correct.

ok, updated to auto install elm when the command isn't available globally.

devongovett · 2018-09-24T04:57:08Z

Updated to auto install elm in the project if the command is not already installed globally. Also calls elm init to create elm.json if one isn't found.

kzc reviewed Aug 31, 2018

View reviewed changes

DeMoorJasper requested changes Sep 2, 2018

View reviewed changes

DeMoorJasper reviewed Sep 2, 2018

View reviewed changes

DeMoorJasper requested changes Sep 2, 2018

View reviewed changes

DeMoorJasper approved these changes Sep 2, 2018

View reviewed changes

devongovett requested changes Sep 22, 2018

View reviewed changes

benthepoet added 17 commits September 23, 2018 21:54

Implemented change so that any asset can force a reload in HMR.

563297e

Added asset for Elm.

7f2d72e

Registered extension.

0386b89

Set Elm assets to force a page reload.

acca76d

Added comment referencing optimal Terser configuration.

fad0f8c

Added integration test for Elm.

2062007

Excluded package folder.

ae839b3

Fixed test.

52e5aac

Fixed path for integration test.

7e44412

Fixed linting issues.

1c1c937

Added test for production mode.

a2127ea

Fixed module path.

a6ec420

Removed assertion.

6844a1b

Fixed assertion for minification test.

51d07ad

Updated comment.

0990345

Removed formatting in GraphQL test.

c1a8e8e

Updated yarn.lock.

ba651ce

benthepoet and others added 13 commits September 23, 2018 21:54

Switched minification to a single pass.

ecfd64a

Fixed minification options.

dcd7d42

Disabled rename pass.

c013178

Added error handling for minification.

4026284

Adjusted error throw.

646dc93

Set compress to perform 2 passes.

28a5823

Fixed linting issue.

03bd986

Renamed flag to be better describe its purpose.

c0766da

Added Elm package file to asset dependencies.

9cb6d70

Added a missing await.

100ffda

Adjusted package version and replaced helper function.

a588ea8

Removed a redundant package.

71358f4

Auto install elm and create elm.json if needed

d856fd6

devongovett force-pushed the elm branch from 292d0b1 to d856fd6 Compare September 24, 2018 04:55

devongovett approved these changes Sep 24, 2018

View reviewed changes

devongovett merged this pull request into parcel-bundler:master Sep 24, 2018

devongovett pushed a commit that referenced this pull request Sep 24, 2018

Add support for Elm assets (#1968)

6ab52e8

benthepoet deleted the elm branch September 24, 2018 11:32

kzc mentioned this pull request Sep 26, 2018

Elm minification is not two-phase #2062

Closed

devongovett pushed a commit that referenced this pull request Oct 15, 2018

Add support for Elm assets (#1968)

70f2981

devongovett pushed a commit that referenced this pull request Oct 15, 2018

Add support for Elm assets (#1968)

948159b

domenkozar mentioned this pull request Jan 2, 2019

Compile Elm with --optimize in production ssuman/parcel-plugin-elm#12

Closed

f-f mentioned this pull request Mar 15, 2019

Integration with Parcel? purescript/spago#134

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Elm assets #1968

Add support for Elm assets #1968

benthepoet commented Aug 31, 2018 •

edited

Loading

kzc commented Aug 31, 2018

benthepoet commented Aug 31, 2018 •

edited

Loading

kzc Aug 31, 2018

kzc Aug 31, 2018

kzc Aug 31, 2018

kzc commented Aug 31, 2018

benthepoet commented Aug 31, 2018

kzc commented Aug 31, 2018

DeMoorJasper left a comment •

edited

Loading

DeMoorJasper Sep 2, 2018

DeMoorJasper Sep 2, 2018

benthepoet Sep 2, 2018

DeMoorJasper Sep 2, 2018

devongovett Sep 22, 2018

devongovett Sep 22, 2018

benthepoet Sep 23, 2018

devongovett Sep 23, 2018

benthepoet Sep 23, 2018

devongovett Sep 24, 2018

devongovett commented Sep 24, 2018

Add support for Elm assets #1968

Add support for Elm assets #1968

Conversation

benthepoet commented Aug 31, 2018 • edited Loading

kzc commented Aug 31, 2018

benthepoet commented Aug 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzc commented Aug 31, 2018

benthepoet commented Aug 31, 2018

kzc commented Aug 31, 2018

DeMoorJasper left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devongovett commented Sep 24, 2018

benthepoet commented Aug 31, 2018 •

edited

Loading

benthepoet commented Aug 31, 2018 •

edited

Loading

DeMoorJasper left a comment •

edited

Loading