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

Add create-yorkie-app #643

Merged
merged 19 commits into from
Nov 18, 2023
Merged

Add create-yorkie-app #643

merged 19 commits into from
Nov 18, 2023

Conversation

se030
Copy link
Contributor

@se030 se030 commented Sep 10, 2023

What this PR does / why we need it?

  • Provides CLI starter kit to allow npm users to conveniently scaffold yorkie-based cooperative apps

Any background context you want to provide?

  • All source codes related to this PR are put in the tools/create-yorkie-app directory

  • create-yorkie-app package needs to be bundled to be published as a binary script package (to be used like npx create-yorkie-app)

    • used webpack bundler which is already used and installed on this project

    • The output bundle includes

      • index.ts script compiled in js
      • resolved packages to provide them in CLI directly from the registry
  • This chunk of work needs to be npm-published independently

  • Referred to create-vite

What are the relevant tickets?

Checklist

  • Added relevant tests or not required
  • Didn't break anything

@CLAassistant
Copy link

CLAassistant commented Sep 10, 2023

CLA assistant check
All committers have signed the CLA.

@krapie
Copy link
Member

krapie commented Sep 11, 2023

[WIP] webpack's importMeta: false option is not working as expected 😅

Any ideas on this issue @chacha912 @mojosoeun @blurfx?

@krapie
Copy link
Member

krapie commented Sep 11, 2023

@se030 also, please sign your CLA!

@blurfx
Copy link
Member

blurfx commented Sep 11, 2023

Can you give us more details on how to reproduce that issue? it doesn't seem to me to be a problem.

I followed these steps:

npm run build
npm pack
npm i -g create-yorkie-app-0.0.0.tgz

…

create-yorkie-app

Copy link
Member

@blurfx blurfx left a comment

Choose a reason for hiding this comment

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

Thank you for great work! but I think we should discuss project structure now. Technically, create-yorkie-app is not part of the js sdk and it sharing a hoisted dependency, which is causing a dependency conflict. And even if we resolve the dependency issue, there could be ghost dependency problem.

So I think we should configuring it to monorepo(workspace) like examples. what do you think? @yorkie-team/maintainers @yorkie-team/mentors @se030

@hackerwins
Copy link
Member

@blurfx Thanks for the suggestion. I also agree with you.

It would be good to move create-yorkie-app to yorkie-js-sdk/tools/create-yorkie-app and configure it as a workspace like examples.

@se030
Copy link
Contributor Author

se030 commented Sep 17, 2023

Can you give us more details on how to reproduce that issue? it doesn't seem to me to be a problem.

I followed these steps:

npm run build
npm pack
npm i -g create-yorkie-app-0.0.0.tgz

…

create-yorkie-app

@blurfx Sorry for the late reply. Does the output file dist/create-yorkie-app.mjs contain import.meta.url? In my case it is converted into file:// path to the original .ts source after npm run build. It becomes a problem when executing the actual output script, not when bundling and publishing it.
I am trying to make the importMeta option work correctly but haven't figured out the solution for now. Guess it would be better to rewrite this config file from scratch.

For the workspace issue, I'll move these sources to yorkie-js-sdk/tools.

@blurfx
Copy link
Member

blurfx commented Sep 18, 2023

@se030

It seems okay to me. this is part of build output of mine, and there is import.meta.url.

Could you try remove node_modules from the entire project, including those in the project root (yorkie-js-sdk), then installing only the dependencies from create-yorkie-app and try again?

// ...
console.log(`\nScaffolding project in ${root}...`);
const templateDir = external_node_path_default().resolve((0,external_node_url_namespaceObject.fileURLToPath)(import.meta.url), `../examples/${template}`);
// ...

@se030
Copy link
Contributor Author

se030 commented Sep 19, 2023

@se030

It seems okay to me. this is part of build output of mine, and there is import.meta.url.

Could you try remove node_modules from the entire project, including those in the project root (yorkie-js-sdk), then installing only the dependencies from create-yorkie-app and try again?

// ...
console.log(`\nScaffolding project in ${root}...`);
const templateDir = external_node_path_default().resolve((0,external_node_url_namespaceObject.fileURLToPath)(import.meta.url), `../examples/${template}`);
// ...

Thanks for your comment.
It is working well after reinstalling node_modules ...😱 must have been npm's dependency issue
I'll organize the work and open this PR as soon as possible.

@blurfx
Copy link
Member

blurfx commented Sep 19, 2023

@se030 I think ghost dependency issue causes this problem. Setting a workspace can solve this problem.

I accidentally closed the PR. I just reopened it. Sorry!

@blurfx blurfx closed this Sep 19, 2023
@blurfx blurfx reopened this Sep 19, 2023
- add build script for create-yorkie-app
- webpack.config) remove unnecessary output.library option and clarify output type as module
@se030
Copy link
Contributor Author

se030 commented Sep 26, 2023

By Create tools/ npm workspace and move create-yorkie-app under it commit there are two workspaces in yorkie-js-sdk.

create-yorkie-app is closely tied to the example apps and its build script copies the whole examples/ directory into output. To me, it seems better to have these source codes under examples/ like below and wanna ask for opinions.

image

This way we can remove (tiny but) unnecessary copy process and users/contributors may also recognize what does create-yorkie-app provide more easily.

Any opinions on this suggestion?

@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (01ddd1a) 67.92% compared to head (00612fb) 67.94%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #643      +/-   ##
==========================================
+ Coverage   67.92%   67.94%   +0.02%     
==========================================
  Files          58       58              
  Lines        8747     8783      +36     
  Branches      788      795       +7     
==========================================
+ Hits         5941     5968      +27     
- Misses       2547     2555       +8     
- Partials      259      260       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@blurfx
Copy link
Member

blurfx commented Sep 28, 2023

First, there's something we need to know:
When building the Yorkie homepage, it pulls examples from the JS SDK to create static pages.

For example, the file tree and example code section on the left side of profile stack example page are statically generated from examples in the JS SDK.

image

So my opinion is:
I think it would be better to locate create-yorkie-app and examples on the same level. because examples are still an independent project and are not completely dependent on create-yorkie-app.

FYI: build:examples script is for creating an js sdk api reference page and you can find the link to api reference here.

@se030 se030 force-pushed the create-yorkie-app branch from 06fe482 to a430de3 Compare October 15, 2023 04:55
Copy link
Member

@blurfx blurfx left a comment

Choose a reason for hiding this comment

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

Thank you. I've left a couple of comments.

tools/create-yorkie-app/index.ts Outdated Show resolved Hide resolved
tools/create-yorkie-app/index.ts Outdated Show resolved Hide resolved
tools/create-yorkie-app/frameworks.ts Outdated Show resolved Hide resolved
@se030 se030 requested a review from blurfx November 14, 2023 11:36
Copy link
Member

@blurfx blurfx left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. Great work 👍

Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.
I left comments about how to use it and how to test it.

tools/create-yorkie-app/README.md Show resolved Hide resolved
@se030 se030 requested a review from blurfx November 18, 2023 12:56
@blurfx blurfx requested a review from hackerwins November 18, 2023 13:01
Copy link
Member

@blurfx blurfx left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for your contribution :)

Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

@hackerwins hackerwins merged commit 07822f5 into yorkie-team:main Nov 18, 2023
hackerwins pushed a commit that referenced this pull request Nov 18, 2023
This commit introduces CLI starter kit, enabling users to easily
scaffold Yorkie-based cooperative applications. The goal is to provide
a convenient way for users to kickstart their projects using Yorkie.

All source codes related to this pull request have been organized
within the tools/create-yorkie-app directory.
@hackerwins
Copy link
Member

hackerwins commented Nov 18, 2023

I merged the PR to test the create-react-app publish GitHub Action. However, it seems to be failing in CI. It might be necessary to register create-react-app on npm. To fix this, I commented out the publish script, but there is another issue related to zlip.
https://github.com/yorkie-team/yorkie-js-sdk/actions/runs/6914576520/job/18812550954#step:7:24

Additionally, We need to think about version policies considered with yorkie-js-sdk version and its dependencies in the scaffolding.

@blurfx @se030 Can you help us create a branch in the origin repository rather than the forked repository? We can easily fix the problems together.

@blurfx
Copy link
Member

blurfx commented Nov 18, 2023

@hackerwins Do you mean creating a branch for testing in this repository?

@hackerwins
Copy link
Member

@blurfx Yes.

@blurfx blurfx mentioned this pull request Nov 18, 2023
2 tasks
@blurfx
Copy link
Member

blurfx commented Nov 18, 2023

I've created create-yorkie-app branch from @se030's repository.

@blurfx blurfx mentioned this pull request Nov 18, 2023
2 tasks
@se030
Copy link
Contributor Author

se030 commented Nov 19, 2023

Additionally, We need to think about version policies considered with yorkie-js-sdk version and its dependencies in the scaffolding.

This source code should not require much patch fixes (I hope), and there exists a chained dependency like yorkie-js-sdk - /examples - create-yorkie-app.

How about going with policy like this? (npm follows semantic versioning)

image

(Additional opinion) Each sub projects(examples and create-yorkie-app) needs to be manually updated for now, but guess we can build a better workflow along with monorepo setup.

@hackerwins
Copy link
Member

This source code should not require much patch fixes (I hope), and there exists a chained dependency like yorkie-js-sdk - /examples - create-yorkie-app.

@se030 Could you explain the dependency between examples in JS SDK and create-yorkie-app? I think I can understand by knowing the way to fetch examples in create-yorkie-app. If it has dependencies the version should match both, if it is not, it is ok that create-yorkie-app has its own version.

],
"scripts": {
"build": "webpack --config ./webpack.config.js && npm run build:copy-assets",
"build:copy-assets": "cp -r ../../examples ./dist && cp ./.env ./dist/.env",
Copy link
Contributor Author

@se030 se030 Nov 20, 2023

Choose a reason for hiding this comment

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

Could you explain the dependency between examples in JS SDK and create-yorkie-app? I think I can understand by knowing the way to fetch examples in create-yorkie-app.

@hackerwins As you can see in this build script, the bundled package shd include examples directory to provide functionality from npm registry (like npx create-yorkie-app). So updating the source codes of example projects would require version update of the package.

Since updating yorkie-js-sdk version for examples/ projects is instructed to be done manually, the following version update should also be done manually (seems nice to add this instruction to MAINTAINING.md).

If it has dependencies the version should match both, if it is not, it is ok that create-yorkie-app has its own version.

Can you give more explanations about this? I'm not sure if information above is enough.


@blurfx By the way, I found this build:copy-assets script and .env file under create-yorkie-app directory are no longer used. We can remove the .env file and modify package.json as:

"scripts": {
  "build": "webpack --config ./webpack.config.js && cp -r ../../examples",
  "start": "npm run build && node dist/create-yorkie-app.mjs"
},

Sorry that I haven't cleaned up well. I tried to add commits to #690 but permission was denied. Could you please reflect these changes?

Copy link
Member

Choose a reason for hiding this comment

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

Now that you have permission, could you push commits to #690?

blurfx pushed a commit that referenced this pull request Nov 25, 2023
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.

[suggestion] Create create-yorkie-app
5 participants