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 Inline code block command linker #70

Closed
wants to merge 8 commits into from

Conversation

yathomasi
Copy link
Contributor

The existing inline code block command (eg: dvc add) depend on sidebar.json on the current project. But, other projects would not include the sidebar.json for other projects.

With this approach, the latest sidebar.json is downloaded with the script before yarn dev or yarn build from the respective GitHub project main branch. The existing sidebar logic is updated to handle this approach.

Fixes #45

@yathomasi yathomasi requested a review from a team August 31, 2022 08:51
@rogermparent rogermparent temporarily deployed to gatsby-theme-inline-cod-ywa3tk August 31, 2022 08:51 Inactive
Copy link
Contributor

@rogermparent rogermparent left a comment

Choose a reason for hiding this comment

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

I don't love the implementation of sticking a plain node script in our NPM scripts when the site build basically depends on it (same with DVC images but to a lesser extent).
I think this should be better integrated with Gatsby, at least downloading the files within something like onPreInit so that we can ensure the required data is at least attempted to be fetched no matter where gatsby is called from.

We also need to keep in mind the possibility that we open this theme up for general use in the future. Though the first iteration doesn't have to be generalized/configurable, we should at least try to write it in a way that can be made generalized.

@yathomasi
Copy link
Contributor Author

I think this should be better integrated with Gatsby, at least downloading the files within something like onPreInit so that we can ensure the required data is at least attempted to be fetched no matter where gatsby is called from.

I have been wondering about a better place for data fetching and onPreInit seems to be a good idea instead of a node script. I also wanted this part to be integrated into the theme itself, but the implementation I was thinking of would require lots of async refactoring to already implemented sidebar logic.

We also need to keep in mind the possibility that we open this theme up for general use in the future.

The implementation is trying to be general to our use case where we have sidebar.json in GitHub repos. But, we can easily generalize by taking the source of the JSON file instead of the repo and main branch.

const tools = [
  {
    name: 'dvc',
    data: dvcSidebar
  },
  {
    name: 'cml',
    repo: 'cml.dev',
    mainBranch: 'master'
  }
]

@yathomasi
Copy link
Contributor Author

onPreInit would be the right place for data fetching but for this particular case it doesn't seem to fit well.
We have the packages/gatsby-theme-iterative/src/utils/shared/sidebar.js file load before the onPreInit is triggered.
Screen Shot 2022-09-02 at 20 09 05

The way we have set up the sidebar plus(sync operation) and the data fetching being an async operation, it is difficult to fit in the process. I don't think converting this into a source node and consuming from there is also not a good idea. WDYT? cc: @rogermparent

@rogermparent
Copy link
Contributor

The way we have set up the sidebar plus(sync operation) and the data fetching being an async operation, it is difficult to fit in the process. I don't think converting this into a source node and consuming from there is also not a good idea. WDYT?

I actually do think using Gatsby's GraphQL system somehow would be the best option, but it would take a lot of dev work. We can still make a useful iteration by adding a basic data-fetching pattern for external sidebars. The first thing that comes to my mind is a thunk-like implementation that exposes the sidebar values from a module, but delays downloading until onPreInit, or otherwise on first usage.

I think we're definitely going to have to turn a few things async no matter how we go about it, since fetching sidebars from external repos is inherently async. Hopefully we can isolate all of that to build-time, and the site itself can just consume static data.

@rogermparent rogermparent temporarily deployed to gatsby-theme-inline-cod-ywa3tk September 8, 2022 18:53 Inactive
@yathomasi
Copy link
Contributor Author

I actually do think using Gatsby's GraphQL system somehow would be the best option, but it would take a lot of dev work.

I gave another round look into this implementation and it feels like converting the sidebar data into nodes will not only need a lot of dev work but also consuming those nodes also doesn't seem to be efficient.

If we were to use it for simple data rendering it would be a good idea to use the gatsby solution for it. But, we have more than such use case, we have to use the data beyond data rendering and for the command-line, api-linker, etc. I also find it difficult to consume it over gatsby-remark plugins. Also, I think the approach will going to use multiple graphql data-query for same data since it's going to be used on multiple places.

Most of the sidebar logic is here.

My take on this is that let's proceed with the simple straightforward solution for now and review/suggestions for improving the current solution would be a great way to proceed.

Comment on lines +13 to +14
// Or, we can also simply use url to sidebar.json file
// url: 'https://raw.githubusercontent.com/iterative/cml.dev/master/content/docs/sidebar.json'
Copy link
Contributor

Choose a reason for hiding this comment

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

Smart idea! We don't need to host the json file on our website if GitHub's already doing the exact same thing.

@rogermparent
Copy link
Contributor

My take on this is that let's proceed with the simple straightforward solution for now and review/suggestions for improving the current solution would be a great way to proceed.

100%, nitpicks aside you have a very viable solution here that fixes the problem and can be built on later. Nice work!

@rogermparent rogermparent temporarily deployed to gatsby-theme-inline-cod-klyzst October 20, 2022 18:07 Inactive
@rogermparent rogermparent temporarily deployed to gatsby-theme-inline-cod-klyzst October 31, 2022 17:35 Inactive
@yathomasi
Copy link
Contributor Author

Closing as it will require setup changes in all the websites and also its significance decreased as we are moving towards unifying if possible. We can come back anytime later if required.

@yathomasi yathomasi closed this May 5, 2023
@yathomasi yathomasi deleted the inline-code-command-linker branch May 15, 2023 10:57
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.

Generalize Inline code linker
2 participants