Skip to content

Conversation

@ramasilveyra
Copy link
Contributor

@ramasilveyra ramasilveyra commented May 14, 2017

WIP

TODO


Hello, I'm eager to use butternut on apps that uses webpack. This pr fix and adds some things that I see missing while looking the code of babili-webpack-plugin and uglify-webpack-plugin.

Improvements

  • Build with babel: target the same webpack node support
  • Include additionalChunkAssets
  • Add support for sourcemaps
  • Add initial tests

@balthazar
Copy link
Owner

Hey! Thanks for this!

Regarding the multiples patches you propose:

  • I prefer keep using the es2015 preset rather than limiting people to node 4.
  • A single index.js inside a src folder doesn't makes much sense to me, keep it simple
  • Adding additionalChunkAssets sounds good, but I'm not in favor of creating that much overhead just to do a filter.
  • The regex looks good, didn't thought about that
  • Do we actually need to have our own config to specify include and exclude patterns? Just wondering

I'm gonna wait for what Sean as to say about the butternut sourcemap support

@ramasilveyra
Copy link
Contributor Author

ramasilveyra commented May 14, 2017

Thanks for the feedback! This is why I sent the pr so earlier 😄

I prefer keep using the es2015 preset rather than limiting people to node 4.

This was using only transform-es2015-modules-commonjs and in one commit I'm using spread operator (...) and that syntax only works on node v6, not on node v4 (webpack supports node v4 and we should do it too).
I changed to the env preset that will use only the babel plugins needed to transpile to node v4 valid code. That code will work also on newer versions of node. This is also helpful to use almost any ES valid syntax in the tests code (even async await like i'm doing).

A single index.js inside a src folder doesn't makes much sense to me, keep it simple

Yeah makes sense, will change 👍 .

Adding additionalChunkAssets sounds good, but I'm not in favor of creating that much overhead just to do a filter.

Sorry I think that I didn't get it. By that much overhead just to do a filter, if you mean about the performance of using a .filter maybe I can change the code to something like:

        // ...
        const files = getFiles(chunks, compilation)

        for (const file of files) {
          if (!ModuleFilenameHelpers.matchObject(matchObjectOpts, file)) {
            return
          }
          // ...
        }
        // ...

function getFiles (chunks, compilation) {
  const files = []
  chunks.forEach((chunk) => files.push(...chunk.files))
  files.push(...compilation.additionalChunkAssets)
 
  return files
}

Do we actually need to have our own config to specify include and exclude patterns? Just wondering

Yes, webpack/webpack#560 (I will add this link to the commit msg).

I'm gonna wait for what Sean as to say about the butternut sourcemap support

Yes I'm doing the same and also playing with sorcery to see if I can make it work. Having the sourcemaps in the right way it's a must (it's really helpful if you use tools like Sentry).

@ramasilveyra
Copy link
Contributor Author

ramasilveyra commented May 14, 2017

Looking to this webpack/webpack#1079 it seems that include and exclude config patterns it's really confusing, it works only on the post processed chunks (not on every file like loaders). Now I think that it's not worth it, lets keep a minimal api surface area, wdyt?

@balthazar
Copy link
Owner

Yep I agree, if you could make a separate PR containing the change of regex, that would be better for me. If you think giving the ability to the user to change this regex would be helpful, then we can pass it as a second parameter. But do we really need it since butternut only supports js anyway?

@ramasilveyra
Copy link
Contributor Author

Cool I will sent another pr with that. Yes you convince me, you are right, I can't think in a use case for that feature lol

@ramasilveyra ramasilveyra force-pushed the improvements branch 3 times, most recently from 2a782bb to 4ceba83 Compare May 14, 2017 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants