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

Use resolve.modules instead of explicit aliases when adding NPM packages #1077

Merged
merged 4 commits into from
Jan 29, 2024

Conversation

tobiasberge
Copy link
Contributor

@tobiasberge tobiasberge commented Aug 22, 2023

  • Currently our webpack docs tell to use explicit aliases when adding an NPM package, see: https://developer.shopware.com/docs/guides/plugins/plugins/plugin-fundamentals/using-npm-dependencies
  • While this works fine in general, it requirers an alias for each new NPM package.
  • It can also cause problems when the NPM package points to native ES modules directly which will be also bundled by our webpack build (instead of pointing to a pre-bundled JS). Because sub-modules import 'package-name/sub-stuff' within the node_modules code would also need an extra alias.
    • This can lead to strange behavior (some packages work, some do not)

I propose to use resolve.modules instead for apps/plugins and including the entire node_modules folder:

module.exports = (params) => {
    return {
        resolve: {
+            modules: [
+                `${params.basePath}/Resources/app/storefront/node_modules`,
+            ],
-            alias: {
-                'algoliasearch/lite': resolve(
-                    join(__dirname, '..', 'node_modules', 'algoliasearch/lite')
-                ),
-                'instantsearch.js': resolve(
-                    join(__dirname, '..', 'node_modules', 'instantsearch.js')
-                ),
-                'algoliasearch-helper': resolve(
-                    join(__dirname, '..', 'node_modules', 'algoliasearch-helper')
-                ),
-                'hogan.js': resolve(
-                    join(__dirname, '..', 'node_modules', 'hogan.js')
-                ),
-                '@algolia/autocomplete-core': resolve(
-                    join(__dirname, '..', 'node_modules', '@algolia/autocomplete-core')
-                ),
-                // etc...
-            }
        }
    };
}

@tobiasberge tobiasberge added the Improvement PR created to append/modify info in the existing article label Aug 22, 2023
@sushmangupta sushmangupta added the Storefront ct-storefront label Aug 23, 2023
@tobiasberge tobiasberge added the Admin @ct-admin label Aug 23, 2023
@tobiasberge
Copy link
Contributor Author

tobiasberge commented Aug 23, 2023

  • Check what happens when two extensions require the same package with different versions.

I added blocked label until further testing is done.

@tobiasberge tobiasberge added the Blocked Block PRs from merging label Aug 23, 2023
@bojanrajh bojanrajh added DX Team #dev-docs Manual migration (DevHub) This PR needs to be manually migrated after the DevHub content is migrated and removed DX Team #dev-docs labels Sep 8, 2023
@Isengo1989
Copy link
Collaborator

@tobiasberge short follow-up - was the mentioned test successful, or is it outstanding?

image

@bojanrajh bojanrajh changed the base branch from main to v6.5 November 22, 2023 10:42
@bojanrajh bojanrajh changed the base branch from v6.5 to main November 23, 2023 20:59
@tobiasberge
Copy link
Contributor Author

@Isengo1989 Hey, sorry for the long delay. In 6.5.x this had side effects when to apps used the same npm package with a different version. I'll have to re-test it for 6.6 because we migrated to webpack multi compiler. With 6.6 this should be fine / more safe because every App/Plugin gets a separate build process under the hood.

Feel free to close this PR in the meantime if you want to keep the PRs clean. I can re-open or create a new one.

@Isengo1989 Isengo1989 marked this pull request as draft December 7, 2023 15:17
@Isengo1989
Copy link
Collaborator

@Isengo1989 Hey, sorry for the long delay. In 6.5.x this had side effects when to apps used the same npm package with a different version. I'll have to re-test it for 6.6 because we migrated to webpack multi compiler. With 6.6 this should be fine / more safe because every App/Plugin gets a separate build process under the hood.

Feel free to close this PR in the meantime if you want to keep the PRs clean. I can re-open or create a new one.

No worries, as discussed we leave the PR in draft till everything is figured out for v6.6

@Isengo1989 Isengo1989 changed the base branch from main to next-6.6 January 29, 2024 09:27
@Isengo1989 Isengo1989 marked this pull request as ready for review January 29, 2024 09:28
@Isengo1989 Isengo1989 removed the Blocked Block PRs from merging label Jan 29, 2024
@Isengo1989
Copy link
Collaborator

@tobiasberge as discussed in Slack I added the main.js info, updated some grammar and added the build commands. We will merge it in next-6.6 soon (changed the target branch).

@bojanrajh rdy to merge 👋

@bojanrajh bojanrajh merged commit f2c4d61 into next-6.6 Jan 29, 2024
5 of 6 checks passed
@bojanrajh bojanrajh deleted the tobiasberge-patch-1 branch January 29, 2024 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin @ct-admin Improvement PR created to append/modify info in the existing article Manual migration (DevHub) This PR needs to be manually migrated after the DevHub content is migrated Storefront ct-storefront +v6.6
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants