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

Add support for Elm assets #1968

Merged
merged 30 commits into from Sep 24, 2018
Merged

Add support for Elm assets #1968

merged 30 commits into from Sep 24, 2018

Conversation

benthepoet
Copy link
Contributor

@benthepoet benthepoet commented Aug 31, 2018

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
Copy link

kzc commented Aug 31, 2018

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
Copy link
Contributor Author

benthepoet commented Aug 31, 2018

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

unsafe: true,
unsafe_comps: true
},
mangle: true
Copy link

Choose a reason for hiding this comment

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

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

         rename: false,

};

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

Choose a reason for hiding this comment

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

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

};

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

Choose a reason for hiding this comment

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

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

@kzc
Copy link

kzc commented Aug 31, 2018

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
Copy link
Contributor Author

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

kzc commented Aug 31, 2018

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

Copy link
Member

@DeMoorJasper DeMoorJasper left a comment

Choose a reason for hiding this comment

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

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

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

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

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
package.json Outdated
@@ -82,7 +82,9 @@
"codecov": "^3.0.0",
"coffeescript": "^2.0.3",
"cross-env": "^5.1.1",
"elm": "^0.19.0-bugfix2",
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

this.name
);
const dependencies = await findAllDependencies(this.name);
const packageFile = await findPackageFile(path.dirname(this.name));
Copy link
Member

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's correct.

Copy link
Member

Choose a reason for hiding this comment

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

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

@devongovett
Copy link
Member

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.

@devongovett devongovett merged this pull request into parcel-bundler:master Sep 24, 2018
devongovett pushed a commit that referenced this pull request Sep 24, 2018
@benthepoet benthepoet deleted the elm branch September 24, 2018 11:32
devongovett pushed a commit that referenced this pull request Oct 15, 2018
devongovett pushed a commit that referenced this pull request Oct 15, 2018
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.

None yet

4 participants