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

Custom builder and --why #22

Closed
okonet opened this issue Sep 20, 2017 · 21 comments
Closed

Custom builder and --why #22

okonet opened this issue Sep 20, 2017 · 21 comments

Comments

@okonet
Copy link

okonet commented Sep 20, 2017

Running size-limit on react-dropzone throws an error:

$ size-limit --why
 ERROR  Error: 
ERROR in ./src/index.js
Module parse failed: /Users/okonet/Projects/OSS/react-dropzone/src/index.js Unexpected token (266:17)
You may need an appropriate loader to handle this file type.
|   }
| 
|   renderChildren = (children, isDragActive, isDragAccept, isDragReject) => {
|     if (typeof children === 'function') {
|       return children({
 @ multi ./src/index.js
    at runWebpack.then.stats (/Users/okonet/Projects/OSS/react-dropzone/node_modules/size-limit/index.js:150:15)
    at <anonymous>
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Looking at code I could not find babel-loader. Is this project suppose to only be used with ES5 code?

@ai
Copy link
Owner

ai commented Sep 20, 2017

Hm. We use UglifyES specially too parse ES6.

Maybe you have ES2017 here? Do you call Size Limit to uncomplicated sources or on dist/ directory (content which you publish to npm).

@okonet
Copy link
Author

okonet commented Sep 20, 2017

Right now we have dist/index.js there but the report size-limit generates is useless for the dist. I've tried to use src/index.js there in order to get insights for optimization, but it failed with the error.

@ai
Copy link
Owner

ai commented Sep 20, 2017

Understand. I will think about the solution.

Right now I have 2 questions:

  1. Do you pack all your files into one file in dist/? Like, use rollup/webpack in library build process.
  2. Why do you do it and not exports multiple files?

@okonet
Copy link
Author

okonet commented Sep 20, 2017

@ai
Copy link
Owner

ai commented Sep 20, 2017

UMD bundle is something like AMD? To take your library in single file and load it into non-webpack project?

@okonet
Copy link
Author

okonet commented Sep 20, 2017

Yes. It allows non-webpack users to use it.

@ai
Copy link
Owner

ai commented Sep 20, 2017

Other questions (sorry that I use you to understand non-relevant topic :D)

  1. Why not export multiple files for webpack user and umd for non-webpack users?
  2. Do you have many non-webpack users in React community?
  3. What if user need only single file from your npm package? tree-shaking still doesn’t work perfectly right now.

@ai
Copy link
Owner

ai commented Sep 20, 2017

If we want to talk about this issue. I see two possible ways:

  1. Use source map and process dist/*.js
  2. Accept custom webpack config (so your UMD build and Size Limit will be packaged by same webpack config)
  3. Add Webpack Bundle Analyzer to your webpack config (so you will use Size Limit only for UMD bundle size check)

If we will just add babel-loader to Size Limit, Size Limit bundle could have different size compared to dist/ file.

@ai
Copy link
Owner

ai commented Sep 20, 2017

I will think about solution on this weekend.

@ai
Copy link
Owner

ai commented Sep 22, 2017

I will think about it on next week. Sorry, I need more time to release Logux 0.2.

@okonet
Copy link
Author

okonet commented Sep 22, 2017

Don’t be sorry! It’s open source and I don’t expect you to jump on this :)

@asotog
Copy link

asotog commented Oct 13, 2017

I have same issue, my project's javascripts are using spread operator and is throwing error

Module parse failed: /Users/alejandrosoto/Downloads/github-repos/crafter-social-rui/src/js/main.js Unexpected token (8:25)
You may need an appropriate loader to handle this file type.

@ai
Copy link
Owner

ai commented Oct 13, 2017

@asotog @okonet temporary solution:

  1. Use Size Limit on your dest/ files. It is the only way to get real accurate cost of your project to the user.
  2. I don’t recommend to pack everything in one file in dest/. It is extremely rare to use React/etc libraries without webpack or any other builder. And with separated files, your users will be able to require separated files import foo from 'lib/foo' without tree-shaking (that doesn’t work) and etc. The simplest and good way.
  3. If you still need to pack everything in one file (or have the legacy project), add Webpack Bundle Analyzer to your builder.

Right now I have the idea to add source map support Webpack Bundle Analyzer. It will solve all these problems perfectly (you can still use everything-in-one-file bundle and size-limit --why will work). But right now I am working on my slides for React Vienna and ReactiveConf, so I will not be able to start work until Oct 26.

@asotog
Copy link

asotog commented Oct 13, 2017

@ai the sizing validation works fine, but the --why flag is not throwing enough information on port 8888 for dest/ file, only shows one big file (main.js) box

@ai
Copy link
Owner

ai commented Oct 13, 2017

@asotog yeap. This happens because of you server one big file to your users main.js. Is there only one file in dest/?

There are 3 options:

  1. Don’t build your files to one file in dest/. Just compile files by Bable, but don’t use Webpack.
  2. Add Webpack Bundle Analyzer to your build tool, which is used to pack files in dest/.
  3. Wait few months while I find a solution how to add source map support to Webpack Bundle Analyzer.

@ai
Copy link
Owner

ai commented Oct 29, 2017

As a quick solution, you can use custom webpack config #29

But it is not the best solution since we must emulate real user experience when somebody will add your library to project. So I will keep this issue open until I will find time to add source map support to Webpack Bundle Analyzer.

@mikesub
Copy link

mikesub commented Jan 3, 2018

👋 Not sure if it's common use case but it's a valid one so I guess it's worth-mentioning.

I have a lib which is es6+ (a private repo). I don't compile it down to es5 as it's being used in a controlled environment, in my other project, which is webpack-based app. So all compilation goes there (only once). The only way to use size-limit in my lib now is to add babel step there, right?

@ai
Copy link
Owner

ai commented Jan 4, 2018

@mikesub nope, Size Limit should work with any syntax supported by Node.js (we use uglify-es which support ES6+). Do you have an error? Maybe there is a problem in webpack or UglifyES parsers.

@mikesub
Copy link

mikesub commented Jan 4, 2018

Found it. Besides common es6+ stuff I use object rest/spread and it's in Stage 3 currently. Removed that and it worked. Thanks!

@zanonnicola
Copy link

I have the same problem with the spread operator. Ended up using the solution suggested by @ai

Don’t build your files to one file in dest/. Just compile files by Bable, but don’t use Webpack.

@ai ai changed the title Support babel by default Custom builder and --why Feb 23, 2018
@ai
Copy link
Owner

ai commented Aug 11, 2019

Size Limit 2.0 was released with modular architecture. Now we have a better UI to explain that --why will not work for a custom builder.

@ai ai closed this as completed Aug 11, 2019
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

No branches or pull requests

5 participants