search

PrestaShop / hummingbird

80 stars 76 forks source link

Removed .env configuration #629

Closed nicosomb closed 3 months ago

nicosomb commented 4 months ago
Questions Answers
Description? Removed .env configuration
Type? improvement
BC breaks? no
Deprecations? no
Sponsor company PrestaShop SA

This PR allows to fix the issue I have here https://github.com/PrestaShop/PrestaShop/pull/36198#issuecomment-2122042886 to embed HB in PrestaShop core.

nicosomb commented 4 months ago

I know that some variables will remain empty, but the build seems ok like this.

Removing .env dependency will help us to ship hummingbird in prestashop core.

nicosomb commented 4 months ago

I know that there were some reasons ... but we can't find them.

Hlavtox commented 4 months ago

PR that introduced this, with discussion - https://github.com/PrestaShop/hummingbird/pull/67

nicosomb commented 4 months ago

OK so it seems that you warned about this issue...

Oksydan commented 4 months ago

@nicosomb .env file is being used by webpack-dev-server mostly to improve DX. Why would you like to get ride of this like this 🤔.

nicosomb commented 4 months ago

@Oksydan I understand, but can you tell us how can we ship it in PrestaShop core please ?

We want to add Hummingbird as a dependency in composer.json (see my PR here https://github.com/PrestaShop/PrestaShop/pull/36198). So we have to know how to fill the .env file during the assets build process.

Oksydan commented 4 months ago

Hmmmmm we could pass arguments in the compilation process. If the process is building a theme, we could simply not check if the .env file exists, otherwise check for its existence and terminate the process. For missing values from env, we could set default values:

const {
  PORT: port = null,
  PUBLIC_PATH: publicPath = null,
  SERVER_ADDRESS: serverAddress = null,
  SITE_URL: siteURL = null,
} = process.env;

THB we do not need these values in the compilation process. It is only required for the development process.

nicosomb commented 4 months ago

@Oksydan thank you for the details. I will improve my PR.

nicosomb commented 3 months ago

@Hlavtox we have to go forward, could you please remove your request approval? if necessary, we can still open a new PR later.

florine2623 commented 3 months ago

Hello @nicosomb ,

What are the steps to reproduce ? Do we just need to make sure the theme is well installed ?