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

Add initial webpack config #55

Merged
merged 3 commits into from
Nov 14, 2019
Merged

Add initial webpack config #55

merged 3 commits into from
Nov 14, 2019

Conversation

kristoforsalmin
Copy link

@kristoforsalmin kristoforsalmin commented Nov 7, 2019

So here's the initial webpack config. It's still missing a lot of things, but for now following needs from #54 were addressed:

  • gulp replaced with webpack
  • Multiple configs via webpack-merge
  • Multiple Browserslist environments (modern and legacy browsers)
  • assets-manifest.json is being generated
  • .env is loaded via dotenv (there's also cross-env for Browserslist)
  • core-js updated
  • node-sass replaced with sass

@mi2oon I'm keeping to our present structure with Yeoman for the time being.

That's it so far 🙂 Let me know what you think

@kristoforsalmin kristoforsalmin added this to the v5.0.0 milestone Nov 7, 2019
@kristoforsalmin kristoforsalmin requested review from mi2oon and a team November 7, 2019 12:24
"lint": "npm run gulp scripts:lint styles:lint",
"watch": "npm run gulp watch",
"test": "echo \"Error: no test specified\" && exit 1"
"build:legacy": "cross-env BROWSERSLIST_ENV=legacy webpack",
Copy link
Contributor

Choose a reason for hiding this comment

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

Split code for modern and legacy browsers is a great thing but it adds some inconveniences to the developing process:

  1. The decision which bundle to use should be made on the backend, probably by checking the user agent. I'm not sure if each project is ready to implement this extra logic. And I guess we currently don't have a solution from the box.
  2. If we use legacy polyfilled code by default during the development, we need to check the modern code as well in order to make sure it works the same way. That's kinda extra work :)
  3. Polyfilled files are outputting to the folder named legacy, which is a common way to name bundle for old browsers, but also might be confused with "legacy" code in the project if it has some :)

Probably we'd better make a code-splitting optional if it is possible, what do you think? :)

Copy link
Author

Choose a reason for hiding this comment

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

@sme12 yeah, I also been thinking whether we need to have this sort of setup by default...

  1. There's also this <script nomodule> approach. It requires a bit of JS for browsers that don't understand nomodule attribute. Frankly, I need to revisit this topic to see if it makes more sense.
  2. That's true. Even though one could argue that you mostly do development in your favorite browser and then still have to test in, say, IE afterwards. But yes, it adds up this extra step of npm run build. In a traditional setup you can go away with the results yielded by already running npm start.
  3. legacy/modern words in context of bundling are somewhat common, for example here and here, so I just excused myself from another naming task 😁 However, I see your concern, among other reasonable choices I found were classic/module as seen here (although they convey a slightly different meaning compared to the current solution) and simply old/new (I also like that both words have equal number of characters 🙂).

@kristoforsalmin
Copy link
Author

kristoforsalmin commented Nov 14, 2019

@mi2oon @sme12 I'm going to merge this one, so it isn't up in the air for too long. Let's continue our conversation about the code splitting in #54 😉

@kristoforsalmin kristoforsalmin merged commit ff5a368 into kraftvaerk:next Nov 14, 2019
@kristoforsalmin kristoforsalmin deleted the initial-webpack branch November 14, 2019 09:10
@kristoforsalmin kristoforsalmin self-assigned this Jan 9, 2020
Copy link
Member

@mi2oon mi2oon left a comment

Choose a reason for hiding this comment

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

Looking good.. lets keep the ball rolling 🐤

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

Successfully merging this pull request may close these issues.

3 participants