kraftvaerk / generator-rammevaerk

Scaffold a web project in kraftvaerk style
MIT License
5 stars 4 forks source link

Update Babel settings #4

Closed kristoforsalmin closed 6 years ago

kristoforsalmin commented 6 years ago

Hi there,

I've noticed few things about the way we set up Babel and I have few ideas:

  1. I see we're currently using babel-preset-es2015 and babel-preset-env at the same time, is there a reason for that? I mean, is it some kind of fallback or we can just use babel-preset-env?
  2. Set modules property to false because webpack is doing the same thing afterwards. Just to speed things up.
  3. Do you think we should use babel-plugin-syntax-dynamic-import by default? It's in stage 3 now, so must be pretty safe to use it and webpack supports it out of box. We'll probably need to be extra careful about the way we set it up (permanent chunk naming, etc.).
  4. Remove comments property since it's true by default.

What do you think? I hope they make some sense.

As a side question, do you prefer to keep Babel's configuration in webpack.config.js or .babelrc?

Thanks

mi2oon commented 6 years ago
  1. They are both used mainly because the current version of babel-preset-env doesn't support babel-polyfil but it's there as a preparation for the future. There is an improvement coming in the next version of babel. which will make it possible to do the following and remove the babel-polyfill import as far as I understand.
{
  "presets": [
    ["env", {
      "useBuiltIns": true,
      "targets": {
        "browsers": ["last 2 versions", "safari > 8", "not ie < 11"]
      }
    }]
  ]
}
  1. I think it's just remains from the old setup where I wanted to have the modules transformed into systemJs format. This is surely not needed anymore so if it speeds up things a little lets change it to false.

  2. I've been using dynamic imports on two projects already and as you mentioned it's pretty safe to use them. At least I haven't run into anything. I don't see introduction of dynamic imports as a problem but one should consider when to use this feature and remember chunk naming along which chunkhashing and so on. However, as I see we don't need babel-plugin-dynamic-import or do we? I thought import() was supported out of the box now. Lets add dynamic imports and make sure if syntax is supported. Eslint also needs a little config update to support this.

  3. Just remove it, I think I just added it to explicitly tell who was responsible for magically removing the comments.

bonus questions: I prefer to have webpack.config.js as small and simple as possible so it would be great to use .babelrc for babel configuration as we did before webpack was introduced. I had some trouble with webpack-babel loader which didn't want to read it. So, adding it as a part of the webpack config was just a temporary solution till we figure why it wasnt working. ;)

kristoforsalmin commented 6 years ago

Thanks for the very descriptive answer 😄

I also noticed we have those two guys sitting in the Babel's config:

minified: true,
compact: true,

I didn't really dig into that yet, but they seem a bit redundant since we run UglifyJsPlugin afterwards.

Maybe I missed something... I was just thinking, if we're going to keep Babel's config out of webpack.config.js (I prefer to do it this way too), then we cannot conditionally enable or disable minified or compact based on, let's say, process.env.NODE_ENV === 'production'. I usually don't run things like that during development.

What do you think? Do we plan to conditionally minify output?

Personally, I'd remove minified and compact from Babel's config and run UglifyJsPlugin only in production mode. But as I said, I'm still new into the rammevaerk, so I might have missed something 😄

mi2oon commented 6 years ago

No problem, glad to be able to shed some light and share things I've basically only in my head 😂

I would probably also remove them as you are right this is a job best left to UglifyJsPlugin and furthermore redundant means removal 😉

kristoforsalmin commented 6 years ago

@mi2oon I was thinking maybe we should create a separate issue for the babel-plugin-syntax-dynamic-import thing, so we can close this one (once we merge browserslist-babel to master or just close it now?). What do you think?

mi2oon commented 6 years ago

@racse1 I think just merge and close this one and create a new issue regarding dynamic imports.

kristoforsalmin commented 6 years ago

@mi2oon Is it OK to merge now or we should wait for Babel's release?

mi2oon commented 6 years ago

@racse1 Nah, keep it bleeding edge :P Lets just merge. Furthermore, I don't see any major things coming it the next version looks quite stable.