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

feat: Minification and more #14

Merged
merged 10 commits into from
Aug 2, 2020
Merged

feat: Minification and more #14

merged 10 commits into from
Aug 2, 2020

Conversation

privatenumber
Copy link
Owner

@privatenumber privatenumber commented Aug 1, 2020

Issue

#2

Changes

  • Refactored the organization of src files by splitting up the plugin, loader, and minify plugin into separate files

  • Loader changes

    • BC Source-map option now uses this.sourceMap instead of reading from options. This flag is toggled on by Webpack if devtool uses source-map or if SourceMapDevToolPlugin is used
    • BC loader is now passed in. I think it makes more sense for the user to set this, especially in cases where there's some Webpack magic happening and it's hard to detect the actual file type (eg. .vue files). With this change, a user can configure esbuild-loader more predictably:
      [
        {
          test: /\.js$/,
          loader: 'esbuild-loader',
        },
        {
          test: /\.tsx$/,
          loader: 'esbuild-loader',
          options: {
            loader: 'tsx'
          }
        },
        {
          test: /\.json$/,
          loader: 'esbuild-loader',
          options: {
            loader: 'json'
          }
        },
      ]
  • Added MinifyPlugin

    • Minifies files async (parallel)
    • Source-map setting reads from devtool or sourcemap option. Plugins don't seem to have access to Webpack's sourceMap flag
  • Tests

    • Virtual file-system for testing
    • Minify tests
    • Loader tests (loaders, targets, source-maps)
  • README.md update TBD. I understand if this PR is not mergable considering these are dramatic changes with BCs. But if they're mergable, I can update the README to reflect that, otherwise, I'll re-package it.

Copy link
Contributor

@egoist egoist left a comment

Choose a reason for hiding this comment

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

LGTM 👌🏻

@privatenumber
Copy link
Owner Author

Cool, I updated the README!

Btw, can I help maintain this project?

@egoist
Copy link
Contributor

egoist commented Aug 1, 2020

Sure, just added you as a collaborator.

@privatenumber
Copy link
Owner Author

privatenumber commented Aug 1, 2020

Thanks :)

I can merge this now if you'd like, but I don't think I'll be able to cut a release because of npm.

Edit:

I see you have semantic release setup.
I'm not familiar with how to it yet, but I believe I have to mark this as a BC so it does a major release.
Will look into it tomorrow.

Are you okay with me releasing this whenever?

@egoist
Copy link
Contributor

egoist commented Aug 1, 2020

@privatenumber yes, just write something starting with BREAKING CHANGE: in the commit details when you squash and merge it, then it will receive a major version bump:

Screenshot_20200802-010158__01

@privatenumber
Copy link
Owner Author

Great, thanks!

I'm running some final tests in a few large codebases I'm in. Will release soon 👍

@privatenumber
Copy link
Owner Author

Two findings:

  • The loader causes a strange behavior in my codebase that I haven't been able to pin down. I have assets outputting to js/[name].[chunkhash].en.js, but the [name] gets replaced with a number (guessing asset ID), and the js/ prefix gets repeated when using the loader (eg. /js/js/0.d32a0b797e13ff4d9fff.en.js). I dug into this but there didn't seem anything wrong with esbuild-loader -- I think it's a Webpack bug:

    • If I return the source code given (unaltered) via async callback, it works fine
    • I inspected the transpiled code given by esbuild, but nothing stuck out as being problematic

    Might look into it more later.

  • Doing the transpilation at the end with the minifier yields better results. Minor but definitely in terms of size, probably because the polyfills aren't repeated for every file that needs it. It might be worth just making this a plugin.

I'm okay with merging this for now and addressing these points later. WDYT?

@privatenumber
Copy link
Owner Author

privatenumber commented Aug 2, 2020

I figured why the loader was being problematic. It's because esbuild strips comments even when not minifying, which strips Webpack magic comments.

-main: () => import(
-/* webpackChunkName: "chunk-name" */
-'../chunk-name'
-),
+main: () => import("../chunk-name")

Edit:
Submitted a feature request here evanw/esbuild#309

@privatenumber
Copy link
Owner Author

Going to move forward with this because it's an external issue and this isn't a regression.

@privatenumber privatenumber merged commit 8e1e6b9 into privatenumber:master Aug 2, 2020
@github-actions
Copy link

github-actions bot commented Aug 2, 2020

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@privatenumber privatenumber deleted the egoist-pr branch August 2, 2020 19:57
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.

2 participants