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

New webpack config #67

Merged
merged 10 commits into from
Oct 12, 2021
Merged

New webpack config #67

merged 10 commits into from
Oct 12, 2021

Conversation

Oksydan
Copy link
Contributor

@Oksydan Oksydan commented Oct 8, 2021

Questions Answers
Description? New webpack config. More desc soon I will try to document it. It ain't finished yet. Commands npm run build, npm run watch/npm run dev are working but I am not done with it yet.
Type? improvement
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #31.
How to test? It should compile files. I think @NeOMakinG should be able to test it and approve. No QA needed here.
Possible impacts? Everything should be working as before but with more features.

I am going to test it and fix some issues.
Known issues:

  • CleanWebpackPlugin isn't working - replaced with https://webpack.js.org/configuration/output/#outputclean
  • remove swiper script,
  • bundleAnalizer will receive its own npm script with build to don't break CI/CD that may relay on that build script,
  • have to test esbuild-loader with ts files,
  • add ForkTsCheckerWebpackPlugin to check TS types.
  • publicPath requires discussion

@NeOMakinG It is working and compiling you are able to test it atm and provide some feedback.

To make it working copy .env-example and create .env in the same directory. Fill values depending on your local prestashop installation.

@NeOMakinG
Copy link
Contributor

NeOMakinG commented Oct 11, 2021

@Oksydan just remove swiper related things, we won't use it, it was a miss.

You need to add ForkTsCheckerWebpackPlugin, overwise esbuild-loader is not type checking, it's only transpiling TS files into JS without taking care of types

Overwise it looks fine to me, made me remember that we should split page-related bundles and find a way to import these on every single pages

@NeOMakinG
Copy link
Contributor

We've a problem, looks like fonts are not anymore loading, there should be a miss with css-loader or font-loader I think so

@Oksydan
Copy link
Contributor Author

Oksydan commented Oct 11, 2021

@NeOMakinG I had added you feedback to problems list.
I will take a look at this today evening. It should be done today.

@Hlavtox
Copy link
Contributor

Hlavtox commented Oct 11, 2021

@Oksydan publicPath is not defined, not using .env file

@Oksydan
Copy link
Contributor Author

Oksydan commented Oct 11, 2021

@Hlavtox yes, since publicPath is defined inside env file it will be undefined.
Should we inform inside CLI about not existing .env file and exit from executing script?

@Hlavtox
Copy link
Contributor

Hlavtox commented Oct 11, 2021

@Oksydan Can we hardcode "../" when only wanting to build the assets? It works fine otherwise, except the fonts and img paths. :-)

@NeOMakinG
Copy link
Contributor

@Hlavtox yes, since publicPath is defined inside env file it will be undefined. Should we inform inside CLI about not existing .env file and exit from executing script?

I think that would be a thing yes, informing is the key

@Oksydan
Copy link
Contributor Author

Oksydan commented Oct 11, 2021

@NeOMakinG

  1. Cleaning assets folders added.
  2. Swiper slider removed.
  3. Package.json cleared from unused dependencies.
  4. New script for building with bundleAnalyzer npm run build-analyze
  5. ForkTsCheckerWebpackPlugin added, typescript checked and working properly (I am exited about typescript)
  6. If .env file not existing we got nice looking error msg. Link and text will be changed later.
    Zrzut ekranu 2021-10-11 o 21 51 04
  7. We have to discuss about that publicPath requirements.

@Hlavtox
Copy link
Contributor

Hlavtox commented Oct 12, 2021

@Oksydan @NeOMakinG

We have to discuss about that publicPath requirements.

When you build it and check theme.css, it builds the paths like url(/themes/theme-refacto/assets/fonts/057cc3c927dc0b2e8dbb739a306bd3a3.otf).

The paths must be relative or it won't work in different subfolder or theme folder. url(../fonts/057cc3c927dc0b2e8dbb739a306bd3a3.otf) - this is how it's done in every other theme including classic.

@kpodemski
Copy link
Contributor

@Hlavtox @NeOMakinG

if you have a shop in the subfolder, you need to define a different public path, and that's it, this is why @Oksydan is using .env files 🤔

Scenario

  1. local installation domain: myshop.dev
  2. staging = staging.myshop.com
  3. production: mydomain.com/shop/

It's a matter of having proper .env files, right?

You also have the possibility to create a bundle with overridden ENV variables via CLI.

@NeOMakinG
Copy link
Contributor

I didn't test this yet, I need to do it today in order to confirm that this works with a vhost and without, that's it :p

@Hlavtox
Copy link
Contributor

Hlavtox commented Oct 12, 2021

@kpodemski So everyone who wants to use this theme needs to configure his build environment and build it? WTF :D

How do you want to distribute it?

@kpodemski
Copy link
Contributor

@Hlavtox

that's a good point. I was thinking from the developer perspective :trollface:

@Oksydan
Copy link
Contributor Author

Oksydan commented Oct 12, 2021

@NeOMakinG it is't going to work w/o vhost.
We can stay to relative path but we will not be able to use dynamic import.
Existence of forked version for devs makes sens now 😄

@NeOMakinG NeOMakinG changed the title [WIP] new webpack config New webpack config Oct 12, 2021
Copy link
Contributor

@NeOMakinG NeOMakinG left a comment

Choose a reason for hiding this comment

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

This PR has a lot of benefits:

  • Esbuild speed
  • Nice refacto of our webpack configuration
  • Reloading on change

It does work on my env: without a vhost, using this configuration:

image

Only one thing left: HMR on a non-vhost env

@NeOMakinG
Copy link
Contributor

@Hlavtox

@Oksydan kicked publicPath on build, so it shouldn't be a problem atm

@NeOMakinG NeOMakinG merged commit a087ed5 into PrestaShop:develop Oct 12, 2021
rodriciru pushed a commit to rodriciru/hummingbird that referenced this pull request Nov 17, 2021
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.

[Dev Tools] Refacto Webpack configuration
5 participants