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 - updated way of handling the package generation. #168

Conversation

pszczesniak
Copy link
Contributor

@pszczesniak pszczesniak commented Jun 10, 2024

Suggested merge commit message (convention)

Type (scope): Message. Closes ckeditor/ckeditor5#16543.


Additional information

After merge to branch cc/6190-update-package-generator please remember to update the commit message in this PR:

Feature (generator): New flag --installation-methods that should allow to generate package with current installation methods of CKEditor 5 or with current and legacy methods with DLLs. See ckeditor/ckeditor5#15502, ckeditor/ckeditor5#15739


Link to guide should be added.

Supported scopes

@@ -195,13 +193,18 @@ function startDevelopmentServer( cwd ) {
sampleServer.stdout.on( 'data', data => {
const content = stripAnsiEscapeCodes( data.toString() ).slice( 0, -1 );
const endMatch = /webpack \d+\.\d+\.\d+ compiled successfully in \d+ ms/.test( content );
const errorMatch = content.indexOf( 'ERROR' ) !== -1;
Copy link
Contributor Author

@pszczesniak pszczesniak Jun 11, 2024

Choose a reason for hiding this comment

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

When there are some errors in sample/* files, after the server starts, there is no information what is going.

I've added a naive error handler it; helped me few times while debugging.

@pszczesniak pszczesniak marked this pull request as ready for review June 11, 2024 11:42
@@ -66,6 +68,18 @@ module.exports = async function init( packageName, options ) {
'Example: ' + chalk.gray( packageManager + ' run start' ),
''
].join( '\n' ), { startWithNewLine: true } );

Copy link
Member

Choose a reason for hiding this comment

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

In order to avoid changing the box manually in the future when we want to change displayed text, please use the table package in order to generate the box dynamically.

Copy link
Member

@przemyslaw-zan przemyslaw-zan Jun 18, 2024

Choose a reason for hiding this comment

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

An example:

const tableData = [
	[ '' ],
	[ 'Supporting a wider range of CKEditor 5 versions requires using' ],
	[ 'a more complex method of importing modules from CKEditor 5.' ],
	[ '' ],
	[ 'Read more here: <LINK TO THE GUIDE EXPLAINING THIS></LINK>' ],
	[ '' ]
];

const tableConfig = {
	columns: [ {
		paddingLeft: 2,
		paddingRight: 2
	} ],
	singleLine: true,
	border: {
		topLeft: '╭',
		topRight: '╮',
		bottomLeft: '╰',
		bottomRight: '╯',
		topBody: '─',
		bottomBody: '─',
		bodyLeft: '│',
		bodyRight: '│'
	}
};

logger.info( table( tableData, tableConfig ), { startWithNewLine: true } );

Copy link
Member

Choose a reason for hiding this comment

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

IMO the cost of maintaining this is low, and the table plugin comes with 17 other dependencies. That's quite a lot of dependencies for drawing a table.

https://npmgraph.js.org/?q=table

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.

Update: I did not use the --dev flag, with it, it works as expected.

TS packages do not work:

Options: yarn, TS, Current and legacy methods with DLLs.
Result: Error thrown while creating the package.

Options: yarn, TS, Current methods
Result: Error thrown while running yarn start in the created package.

ERROR in ./src/index.ts 3:0-50
Module not found: Error: Can't resolve './gentest.js' in 'D:\Repositories\ckeditor5-gentest\src'
resolve './gentest.js' in 'D:\Repositories\ckeditor5-gentest\src'
  using description file: D:\Repositories\ckeditor5-gentest\package.json (relative path: ./src)
    Field 'browser' doesn't contain a valid alias configuration
    using description file: D:\Repositories\ckeditor5-gentest\package.json (relative path: ./src/gentest.js)
      no extension
        Field 'browser' doesn't contain a valid alias configuration
        D:\Repositories\ckeditor5-gentest\src\gentest.js doesn't exist
      .ts
        Field 'browser' doesn't contain a valid alias configuration
        D:\Repositories\ckeditor5-gentest\src\gentest.js.ts doesn't exist
      .js
        Field 'browser' doesn't contain a valid alias configuration
        D:\Repositories\ckeditor5-gentest\src\gentest.js.js doesn't exist
      .json
        Field 'browser' doesn't contain a valid alias configuration
        D:\Repositories\ckeditor5-gentest\src\gentest.js.json doesn't exist
      .wasm
        Field 'browser' doesn't contain a valid alias configuration
        D:\Repositories\ckeditor5-gentest\src\gentest.js.wasm doesn't exist
      as directory
        D:\Repositories\ckeditor5-gentest\src\gentest.js doesn't exist

scripts/ci/verify-build.js Outdated Show resolved Hide resolved
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 👍

@pszczesniak pszczesniak merged commit 06fa2b7 into cc/6190-update-package-generator Jun 19, 2024
2 checks passed
@pszczesniak pszczesniak deleted the new-changes-implementation-in-package-generator branch June 19, 2024 12:34
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.

3 participants