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

Removed .env configuration #629

Merged
merged 3 commits into from
Jul 4, 2024
Merged

Conversation

nicosomb
Copy link
Contributor

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 PrestaShop/PrestaShop#36198 (comment) to embed HB in PrestaShop core.

@nicosomb
Copy link
Contributor Author

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.

Copy link
Contributor

@Hlavtox Hlavtox left a comment

Choose a reason for hiding this comment

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

The env configuration was added for a reason. 🤔 Ping @Oksydan @NeOMakinG

@nicosomb
Copy link
Contributor Author

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

@Hlavtox
Copy link
Contributor

Hlavtox commented May 21, 2024

PR that introduced this, with discussion - #67

@nicosomb
Copy link
Contributor Author

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

@Oksydan
Copy link
Contributor

Oksydan commented May 23, 2024

@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
Copy link
Contributor Author

@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 PrestaShop/PrestaShop#36198). So we have to know how to fill the .env file during the assets build process.

@Oksydan
Copy link
Contributor

Oksydan commented May 27, 2024

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
Copy link
Contributor Author

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

@@ -176,7 +176,7 @@
{/block}
</div>

{if $product.add_to_cart_url && $product.product_type !== 'combinations'}
{if $product.add_to_cart_url}
Copy link
Member

Choose a reason for hiding this comment

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

Why this diff? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

This is related to this PR: #609

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know that. But it seems that it breaks UI tests.

Copy link
Member

Choose a reason for hiding this comment

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

I think that's an issue about UI tests then.
If we merge your diff, we reintroduce the "Add to cart" button for combinations products, and his buggy behaviour in that particular case 🤔

But anyway, I don't block your PR, just I prefer to warn you about this! 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the change is legit, add to cart url should be handled on product lazy array settings. For combination, it's availability depends on prestashop settings (enable add to cart button in listings when product has combinations on/off).

@nicosomb nicosomb force-pushed the remove-env-config branch from 35ac6b6 to 1f94b21 Compare June 27, 2024 14:34
@nicosomb nicosomb force-pushed the remove-env-config branch from 1f94b21 to 103cdde Compare July 2, 2024 08:56
@nicosomb nicosomb closed this Jul 2, 2024
@nicosomb nicosomb reopened this Jul 2, 2024
@nicosomb
Copy link
Contributor Author

nicosomb commented Jul 2, 2024

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

@nicosomb nicosomb requested review from boherm and Hlavtox July 2, 2024 14:29
@nicosomb nicosomb added this to the v0.2.1 milestone Jul 2, 2024
@florine2623
Copy link

Hello @nicosomb ,

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

@florine2623 florine2623 self-assigned this Jul 3, 2024
Copy link

@florine2623 florine2623 left a comment

Choose a reason for hiding this comment

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

Hello @nicosomb ,

Hummingbird is well installed.
Made an order with all types of products, order process is working as expected.

LGTM ✅
Thanks!

@nicosomb nicosomb merged commit 063b4dc into PrestaShop:develop Jul 4, 2024
12 checks passed
@nicosomb nicosomb deleted the remove-env-config branch July 4, 2024 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants