Skip to content

Conversation

@andys8
Copy link
Contributor

@andys8 andys8 commented Oct 14, 2019

Resolves paths to assets in strings in Elm code with webpack.

Example:

img [ src "require:src/assets/logo.svg" ] []

Fixes #363

Documentation not part of this PR.

Resolves paths to assets in strings in Elm code with webpack.

Example:

```
img [ src "require:src/assets/logo.svg" ] []
```

Fixes halfzebra#363
@andys8
Copy link
Contributor Author

andys8 commented Oct 14, 2019

This PR is currently removing noParse: /\.elm$/ and it can impact performance (see andys8/elm-asset-webpack-loader#1).

Building a small project 10 times went from 38682 ms to 40909 ms. Would be interested in more comparisons. Especially on larger elm code bases.

#!/bin/bash
START=$(date +%s.%N)
for i in {1..10}; do
   rm -r build
   ~/.npm-global/bin/elm-app build
   # ./node_modules/.bin/elm-app build
done
END=$(date +%s.%N)
DIFF=$(echo "scale=3; (${END} - ${START})*1000/1" | bc)
echo "${DIFF}"
  • Is there a smart way to use noParse?
  • Opt-in for this feature doesn't really fit the project. Opt out might be possible with overwriting the webpack config.

@halfzebra
Copy link
Owner

While removing noParse: /\.elm$/ is not ideal, I'm suspecting there's no way around it.
Having an asset require other modules implies that the asset is being parser for import statements.

Maybe it's worth investigating optional support for imports?

What do you think?

@andys8
Copy link
Contributor Author

andys8 commented Oct 14, 2019

Maybe it's worth investigating optional support for imports?

Do you have something in mind? Opt-in similar to ELM_DEBUGGER?

@ssbb
Copy link

ssbb commented Oct 14, 2019

My benchmarks on ~50k loc project (total time for 10x):

with noParse (noParse: [/.elm$/]): 116000ms
without noParse: 124000ms
without noParse and elm-asset-webpack-loader actually used in single module: 128000ms

I think the only way to process require is to disable noParse for Elm files. But noParse accepts function with file path as argument (https://webpack.js.org/configuration/module/#modulenoparse) so future performance improvements possible - you can move all asset loading into single file and do not parse all Elm files except this one.

Anyway I think such optimizations should be on end user side.

@andys8
Copy link
Contributor Author

andys8 commented Oct 22, 2019

Options I've in mind:

  • Merge as is
  • Opt-in with environment variable
  • Opt-in with setting (requireAssets: true) in elmapp.config.js (if possible)
  • Opt-out with setting (requireAssets: false) in elmapp.config.js (if possible)
  • Instructions in user guide how to opt-out / tweak performance if necessary by overwriting/setting noParse in webpack config

@halfzebra
Copy link
Owner

I feel like this PR introduces a superior user experience and it makes sense to roll with the proposed changes being "enabled-by-default".

It would stop working if anyone will add noParse in elmapp.config.js, so maybe leaving a comment about that in the user guide would make sense.

How does that sound?

@andys8
Copy link
Contributor Author

andys8 commented Oct 22, 2019

Sounds good to me :) Do you think you can add some instructions to documentation to talk about this feature?

Is it possible to release a "release candidate" (tag next) to verify it in applications?

@halfzebra
Copy link
Owner

I'm leaning towards merging this, I feel like there's not much value in releasing it separately as the estimated chances of bug discovery are quite low with tagged releases.

I'll look into adding docs on this feature when I'll get some time. 👍

Nice work!

@halfzebra halfzebra merged commit fcbaa17 into halfzebra:master Nov 5, 2019
@halfzebra
Copy link
Owner

🎉 This PR is included in version 4.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@andys8
Copy link
Contributor Author

andys8 commented Feb 7, 2020

@halfzebra I think this is currently missing in the documentation. I'm not sure about the wording myself. It would be awesome if you could maybe add a small example in the docs.
https://github.com/halfzebra/create-elm-app/blob/master/template/README.md#adding-images-and-fonts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Require assets (e.g. images) with webpack

3 participants