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

Implemented option to install dependencies using npm #38

Merged
merged 6 commits into from
Sep 29, 2021
Merged

Implemented option to install dependencies using npm #38

merged 6 commits into from
Sep 29, 2021

Conversation

przemyslaw-zan
Copy link
Member

Feature: Implemented option to install dependencies using npm. Closes #29.

@przemyslaw-zan
Copy link
Member Author

przemyslaw-zan commented Sep 28, 2021

I've accidentally removed this comment. Is there any particular reason that this is commented out?

https://github.com/ckeditor/create-ckeditor5-plugin/blob/0c71db4197b991ea86798c2577be12961dc7f276/packages/create-ckeditor5-plugin/lib/index.js#L179-L181

@pomek
Copy link
Member

pomek commented Sep 28, 2021

☝️ I guess the verbose option was not implemented yet, so removing the code is fine.

Copy link
Member

@pomek pomek left a comment

Choose a reason for hiding this comment

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

@pomek
Copy link
Member

pomek commented Sep 28, 2021

Calling the create-ckeditor5-package with --dev and --use-npm modifiers results in a project that does not work as npm creates symlinks to the @ckeditor/ckeditor5-package-tools package, while yarn - copies all files.

But it should not be a problem when all packages are installed from the registry.

@pomek
Copy link
Member

pomek commented Sep 28, 2021

But it should not be a problem when all packages are installed from the registry.

But it is an issue if somebody would improve the tool and use npm instead of yarn.

So, since we have a difference in installing packages using a file system between packages manager, let's try to create an archive from the @ckeditor/ckeditor5-package-tools package and install the package from the archive instead of its directory. I'll handle it.

@pomek
Copy link
Member

pomek commented Sep 29, 2021

The problem is more complicated than I thought => npm/npm#15900.

Installing the package from an archive file works. But…

  • An entry from ckeditor5-foo package.json looks weird:
    "@ckeditor/ckeditor5-package-tools": "file:/Users/pomek/Projects/ckeditor/create-ckeditor5-plugin/packages/ckeditor5-package-tools/ckeditor-ckeditor5-package-tools-0.0.1.tgz",
    
    Is it a problem?
  • What about the created archive? Should it be removed? If so, calling * install in ckeditor5-foo will end with an error.

@pomek
Copy link
Member

pomek commented Sep 29, 2021

Ok, I rejected the idea of creating an archive. I'm unsure what should I do with the created archive file. I don't want to see it in my workspace, but if I remove it, the re-installing packages process in ckeditor5-foo does not work.

Fortunately, I figured that yarn supports creating symlinks when installing from the file: protocol. See: yarnpkg/yarn#3359.

Hence, I'd adjust our tool to use the option. Yarn creates symlinks instead of copying files and install all dependencies correctly. Updating files from the ckeditor5-package-tools directory will be shared with ckeditor5-foo which is good.

Npm has some bugs when installing packages and I treat this as an upstream bug that may not be resolved. Let's mention it in the README - to use yarn when developing the project.

@pomek
Copy link
Member

pomek commented Sep 29, 2021

☝️ I guess the verbose option was not implemented yet, so removing the code is fine.

I found a use case for it. All logs are hidden behind the option.

Without --verbose:

image

With --verbose:

image

@pomek
Copy link
Member

pomek commented Sep 29, 2021

Fortunately, I figured that yarn supports creating symlinks when installing from the file: protocol. See: yarnpkg/yarn#3359.

Unfortunately… yarn does not create binary files for linked packages, hence this idea was also rejected. Let's use the file: protocol for both, npm and yarn. If --use-npm --dev modifiers will be a problem in the future, we will find a solution for the problem.

As for now, let's ignore it.

@pomek pomek merged commit 76f1a44 into master Sep 29, 2021
@pomek pomek deleted the i/29 branch September 29, 2021 13:32
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.

Implement installing packages using npm instead of yarn
2 participants