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

getThemePath() fails when a generated package is located in a mono-repo #132

Open
pomek opened this issue Dec 2, 2022 · 2 comments Β· May be fixed by #175
Open

getThemePath() fails when a generated package is located in a mono-repo #132

pomek opened this issue Dec 2, 2022 · 2 comments Β· May be fixed by #175
Labels
squad:devops Issue to be handled by the Devops team. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@pomek
Copy link
Member

pomek commented Dec 2, 2022

πŸ“ Provide detailed reproduction steps (if any)

  1. Create a mono-repo (and use workspaces).
  2. Inside the mono-repo, generate a new package.
  3. Then, install dependencies.
  4. Then, execute yarn start

βœ”οΈ Expected result

It does not throw any errors.

❌ Actual result

$ ckeditor5-package-tools start
node:internal/modules/cjs/loader:998
  throw err;
  ^

Error: Cannot find module '/Users/pomek/Projects/.../packages/ckeditor5-foo/node_modules/@ckeditor/ckeditor5-theme-lark/package.json'

❓ Possible solution

const packagePath = require.resolve( '@ckeditor/ckeditor5-theme-lark/package.json' );
const packageJson = require( packagePath );
@pomek pomek added squad:devops Issue to be handled by the Devops team. type:bug This issue reports a buggy (incorrect) behavior. labels Dec 2, 2022
@pomek
Copy link
Member Author

pomek commented Dec 2, 2022

The same applies to

// An absolute path to the manifest file that the `DllReferencePlugin` plugin uses for mapping dependencies.
const ckeditor5manifestPath = path.join( options.cwd, 'node_modules', 'ckeditor5', 'build', 'ckeditor5-dll.manifest.json' );

Fix:

// An absolute path to the manifest file that the `DllReferencePlugin` plugin uses for mapping dependencies.
const ckeditor5manifestPath = require.resolve( 'ckeditor5/build/ckeditor5-dll.manifest.json' );

@klonwar
Copy link

klonwar commented Apr 12, 2024

TLDR:
Can be fixed using yarn patch and changing getThemePath function from lib/utils:

const packageJsonPath = require.resolve( '@ckeditor/ckeditor5-theme-lark/package.json');
const packageJson = require(packageJsonPath);
const packagePath = path.join(packageJsonPath, '..');

There are two workarounds:

  1. The first is to disable hoisting for the package:

"packages/ckeditor5-whatever/package.json

"installConfig": {
    "hoistingLimits": "dependencies"
}

But this is very bad as dependencies are duplicated during installation

  1. The second is to use yarn patch

Tried to solve this issue using yarn patch command and applying your fixes. It really works. but still somethig is wrong:
image

Seems like some theme deps still not loaded correctly

UPD:
The reason is that original path.join( cwd, 'node_modules', '@ckeditor', 'ckeditor5-theme-lark' ); results in package dir path, while require.resolve( '@ckeditor/ckeditor5-theme-lark/package.json' ); results in package.json path.

The correct fix will be

module.exports = function getThemePath( cwd ) {
  const packageJsonPath = require.resolve( '@ckeditor/ckeditor5-theme-lark/package.json');
  const packageJson = require(packageJsonPath);
  const packagePath = path.join(packageJsonPath, '..');

  return path.join( packagePath, packageJson.main );
};

P.S. Trying to resolve require.resolve( '@ckeditor/ckeditor5-theme-lark'); gives .../node_modules/@ckeditor/ckeditor5-theme-lark/theme/theme.css so this is not an option

Hope this will be in release someday

@danspam danspam linked a pull request Jul 21, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squad:devops Issue to be handled by the Devops team. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants