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

Update Package Generator for NIM. #166

Open
wants to merge 63 commits into
base: master
Choose a base branch
from

Conversation

pszczesniak
Copy link

@pszczesniak pszczesniak commented Apr 26, 2024

Suggested merge commit message (convention)

Feature (generator): New flag --use-only-new-installation-methods that should allow to generate package without support for DLLs. See ckeditor/ckeditor5#15502, ckeditor/ckeditor5#15739


⚠️ Please do not merge after approved review. ⚠️

ℹ️ After approved review we would like to release alpha version - to make it available for tests. ℹ️

Additional information

  • In templates that are using New Installation Methods ckeditor5 and ckeditor5-premium-features are using nightly,

Supported scopes

Copy link
Member

@przemyslaw-zan przemyslaw-zan left a comment

Choose a reason for hiding this comment

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

The build:dist script in non-legacy TS package using yarn shows some kind of warning:

$ yarn run  build:dist
yarn run v1.22.19
$ node ./scripts/build-dist.mjs
1/2: Generating NPM build...
2/2: Generating browser build...
No name was provided for external module "ckeditor5" in "output.globals" – guessing "ckeditor5".
Done in 5.36s.

Other than that, everything seems to work properly.

packages/ckeditor5-package-generator/bin/index.js Outdated Show resolved Hide resolved
@@ -0,0 +1,16 @@
/**
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 JSON file, not JSON with comments. Should this comment be here? Perhaps we could change the format? That might require some additional packages though. Consider moving such comments to README.md.

Copy link
Author

Choose a reason for hiding this comment

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

Will remove it from here and will mention about it in docs for package-generator and in migration guide.

@@ -68,7 +71,7 @@ function copyTemplate( templatePath, packagePath, data ) {

const processedTemplatePath = templatePath
// Remove sub-directory inside templates to merge results into one directory.
.replace( /^(?:common|js|ts)(?:\\|\/)/, '' )
.replace( /^(?:common|js|ts|legacy-js|legacy-ts)(?:\\|\/)/, '' )
Copy link
Member

Choose a reason for hiding this comment

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

I am really sorry for such nitpicks, but could we change legacy-js/legacy-ts to js-legacy/ts-legacy? It would look nicer in the directory tree
image

Copy link
Author

Choose a reason for hiding this comment

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

Fully agree. Addressed in 9b05427

@pszczesniak
Copy link
Author

The build:dist script in non-legacy TS package using yarn shows some kind of warning:

Yes, it's on out todo list to fix in ckeditor5-dev-build-tools it will be done till the CKEditor5 release with NIM.

Copy link
Member

@przemyslaw-zan przemyslaw-zan left a comment

Choose a reason for hiding this comment

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

LGTM 👍

…--without-legacy-methods' will be generate package with only NIM support.
Copy link
Member

@przemyslaw-zan przemyslaw-zan left a comment

Choose a reason for hiding this comment

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

I think we should rename the cli option, to something like:
--without-legacy-methods => --use-new-installation-methods

The changes work as expected! 🎉

@pszczesniak
Copy link
Author

@przemyslaw-zan we decided to name the flag --use-only-new-installation-methods to make it more explicit.

Copy link
Member

@przemyslaw-zan przemyslaw-zan left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@pomek
Copy link
Member

pomek commented May 28, 2024

Changes from this branch have been released: https://github.com/ckeditor/ckeditor5-package-generator/releases/tag/v2.0.0-alpha.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants