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

Make lib/utils/get-webpack-config-dll.js more flexible outside expected package structure #152

Open
timkelty opened this issue May 11, 2023 · 3 comments
Labels
pending:feedback This issue is blocked by necessary feedback. squad:devops Issue to be handled by the Devops team. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@timkelty
Copy link

timkelty commented May 11, 2023

📝 Provide a description of the new feature

packages/ckeditor5-package-tools/lib/utils/get-webpack-config-dll.js is quite rigid about what directory structure it expects, which makes it difficult to use in an existing webpack ecosystem, or other situations where you may want to use this webpack config in a different path context.

I propose passing more options (defaulting to value that are set now) to support these use-cases.
Options would be:

  • dllName
  • ckeditor5manifestPath
  • translationSourceFilesPattern (for when your package includes other plugins you want translations generated for)
  • Something to differentiate a different PACKAGE_ROOT_DIR from the webpack context (eg monorepo)

Additionally, packages/ckeditor5-package-tools/lib/index.js would export packages/ckeditor5-package-tools/lib/utils/get-webpack-config-dll.js.

What is the expected behavior of the proposed feature?

webpack.config.js

const getDllConfig = require('@ckeditor/ckeditor5-package-tools/lib/utils/get-webpack-config-dll.js');

module.exports = getDllConfig({
  dllName: 'foo',
  ckeditor5manifestPath: require.resolve('ckeditor5/build/ckeditor5-dll.manifest.json'),
});

Given that this change would make the webpack config generic, it may make sense for this to live https://github.com/ckeditor/ckeditor5-dev/blob/master/packages/ckeditor5-dev-utils/lib/builds/getdllpluginwebpackconfig.js which is largely duplicated config, but specific for @ckeditor5/* packages.

Seems like it could also be a win for CKEditor to use the same config in both places with options passed, rather than duplicating.

I'm happy to PR this, but wanted see what everyone thought first.

If you'd like to see this feature implemented, add a 👍 reaction to this post.

@timkelty timkelty added the type:feature This issue reports a feature request (an idea for a new functionality or a missing option). label May 11, 2023
@pomek pomek added the squad:devops Issue to be handled by the Devops team. label Sep 4, 2023
@pomek
Copy link
Member

pomek commented Sep 14, 2023

Hi @timkelty,

Tools and utils in the @ckeditor/ckeditor5-package-tools are designed to work with the CKEditor 5 package generator. This tool has strict requirements and limitations to produce output close to the official CKEditor 5 packages.

I agree that developers can use the tools package in their projects. It would make the package more universal and, perhaps, save time configuring all the webpack stuff.

To understand the scope of changes, I would like to ask you to describe which hardcoded values should be transformed into options. Could you include a short description/explanation of why? It would define the final scope more easily.

@pomek pomek added the pending:feedback This issue is blocked by necessary feedback. label Sep 14, 2023
@timkelty
Copy link
Author

@pomek sure! –

Our Craft CMS plugin creates a DLL compatible build, so that other Craft plugins can add CKeditor plugins.

Currently we just tell (Craft) plugin developers to use the package generator to scaffold their CKE plugin.

That is fine, except that Craft plugins will generally have other built assets, also in a webpack pipeline, that need to be built to a specific location to be able to be loaded: https://github.com/craftcms/commerce/blob/develop/src/web/assets/statwidgets/webpack.config.js

So, to restate the issue:
Due to the nature of the "CKE plugin nested in a Craft plugin", we'll end up with a package.json at the root, but our src and dist assets elsewhere (./web/assets/{bundleName}). So, we want to use all the CKE webpack stuff, but we need to be able to tweak some of the hardcoded assumptions derived from the path.

@pomek
Copy link
Member

pomek commented Oct 9, 2023

@timkelty, the final request, would you describe which options should be customizable again? Or, as you mentioned, you are happy to prepare a pull request. If you want, we can move the discussion to a PR once it is ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending:feedback This issue is blocked by necessary feedback. squad:devops Issue to be handled by the Devops team. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

No branches or pull requests

2 participants