Skip to content
This repository has been archived by the owner on Jul 19, 2021. It is now read-only.

Now sections can have separate .json file for schema. #954

Closed
wants to merge 17 commits into from

Conversation

MikhailRoot
Copy link

Now sections can have separate .json file for schema.

If we have section_name.schema.json file in same directory as section_name.liquid file it will inject it as a {% schema %} for this section while placing it in /dist/sections directory.

It checks if section_name.liquid has inline definition and does nothing if it has it inline.
If section_name.liquid don't have neither section_name.schema.json neither inline {% schema %} declaration in it, it outputs a warning.

The win of this approach is simplifying json editing leading to less errors in schema declaration. Also you have good syntax highlighting in editor of choice for .json files for sure.

#749
#856

Checklist

For contributors:

For maintainers:

  • I have 🎩'd these changes.

This module exports function to inject separately placed `section_name.schema.json` file into respective `section_name.liquid` file.
It checks, if liquid file has already set `{% schema %}` or not, and injects it to result `section_name.liquid` file which will be placed in `dist/sections` directory.
Now sections has ability to have separately placed json files to be used as schema.
The idea is:
We have section files in  `/src/sections/`  like `/src/sections/section_name.liquid` and  to simplify editing of section's schema which is json
We now can create just `/src/sections/section_name.schema.json` file and it'll be injected to same named section liquid file upon build process and be placed  wrapped with right `{% schema %}` tags in `/dist/sections/section_name.liquid`
Same logic can be implemented for Scripts and Styles. But i consider Slate's approach is to separate JS and CSS\SCSS out of sections to be built  with webpack instead of having them inlined in sections, even Shopify supports it.
@ghost ghost added the cla-needed label Jan 18, 2019
@t-kelly
Copy link
Contributor

t-kelly commented Jan 29, 2019

Very cool! Thanks @MikhailRoot. Will give this a review.

@harshal317 This might be helpful for your work on section locales.

@t-kelly t-kelly requested review from t-kelly and harshal317 January 29, 2019 14:27
@t-kelly t-kelly self-assigned this Jan 29, 2019
harshal317 and others added 7 commits January 29, 2019 18:20
…emas-to-sections.js

Co-Authored-By: MikhailRoot <durnevM@mail.ru>
…emas-to-sections.js

Co-Authored-By: MikhailRoot <durnevM@mail.ru>
…emas-to-sections.js

Co-Authored-By: MikhailRoot <durnevM@mail.ru>
…emas-to-sections.js

Co-Authored-By: MikhailRoot <durnevM@mail.ru>
…emas-to-sections.js

Co-Authored-By: MikhailRoot <durnevM@mail.ru>
…emas-to-sections.js

Co-Authored-By: MikhailRoot <durnevM@mail.ru>
…emas-to-sections.js

Co-Authored-By: MikhailRoot <durnevM@mail.ru>
Copy link
Author

@MikhailRoot MikhailRoot 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 checks, and sorry for some typo errors.

@harshal317
Copy link
Contributor

Thank you for checks, and sorry for some typo errors.

Hehe sorry typos were bugging me

@MikhailRoot
Copy link
Author

I've signed Shopify's CLA, but i don't know how to trigger recheck.

@ghost ghost removed the cla-needed label Jan 29, 2019
@harshal317
Copy link
Contributor

I've signed Shopify's CLA, but i don't know how to trigger recheck.

Done! There are some linter errors in the CI check though.

@disantlor
Copy link

Rather than include by convention (matching section name), could you not use this same approach to enable something like {% schema 'section_schema.json' %} with no closing tag, like {% include ... %}

The function could look for the .json file specified in the parameter, and then inject it into the page, wrapped in a real open and closing {% schema %} tag.

That way you could solve the (very very useful) issue brought up in #749

added `{% inject 'file_name_to_inject.json|whatever' %}`  path of file to inject is relative to liquid file we inject to, and is resolved via `path.resolve()`.
NB: approach to insert {% schema %} with a separate file named `section-name.schema.json` will work only if there's no `{% schema %}` tag defined in liquid file, so these approaches will not conflict or overwrite each other.
@MikhailRoot
Copy link
Author

MikhailRoot commented Jan 30, 2019

@disantlor thank you for idea, i've implemented it like {% inject 'file_path_relative_to_file_we_inject_into' %} consider keyword inject can be better as we really inject one file into another also we can inject not just .json files
so now
{% schema %} {% inject 'product.schema.json' %} {% endschema %} works as well.

@MikhailRoot
Copy link
Author

Now with {% inject 'schema-part-file.json' %} working in section's liquid files we can inject parts of json into schema, but be careful it will not check json for validity (shopify should refuse to upload file if schema is broken) so parts of json if split into should be valid parts.
also you can use inject on sections to inline some parts of liquid - if it makes sense.

My original approach by naming convention still works.
More over it first tries to insert it by naming convention and then parses this content with just inserted json to look for {% inject 'relative_path/filename' %} so if you like you would place inject instructions in this section-name.schema.json file to be able to insert partials of json, OR
have multiple inject commands inside {% schema %} { "some":"json", {% inject 'partial_file.json' %} ,"another":"data" {% endschema %} to have json partials if necessary.

@MikhailRoot MikhailRoot reopened this Jan 30, 2019
@MikhailRoot
Copy link
Author

Sorry, closed PR by accident. Reopened it :)

@t-kelly
Copy link
Contributor

t-kelly commented Feb 11, 2019

@MikhailRoot could you confirm that an edit to the schema json file triggers a Webpack rebuild when running yarn start?

@MikhailRoot
Copy link
Author

@t-kelly Yes, changing section_name.schema.json triggers rebuild and uploads changed rebuilt files. (just checked it to make sure).

@t-kelly
Copy link
Contributor

t-kelly commented Feb 15, 2019

Hey @MikhailRoot -- First off, thanks for taking the time and working on this great addition to Slate. As you can see in #856 -- this is something we've thought of and wanted to move on.

Coincidentally, you opened this PR just as @harshal317 started working on it. I had him review your PR and we were have both seriously considered using it and expanding upon it. However, we have both come to the opinion that we would like this functionality to live in its own Webpack plugin, and not depend on CopyWebpackPlugin.

All this to say, expect a PR in the next few weeks from Harshal. We would love your feedback on it and will cc you when its posted!

Also, in the future, as we mention in the CONTRIBUTING.md:

If you're thinking of adding a big new feature, consider opening an issue first to discuss it to ensure it aligns to the direction of the project (and potentially save yourself some time!).

It helps to post that you're intending to work on something so we know what's up and let you know if we're already working on it!

@t-kelly t-kelly closed this Feb 15, 2019
@davidwarrington
Copy link

davidwarrington commented Feb 16, 2019

Would it be possible to add .js file support to this? Being able to import a schema exported from a JS file would open up a lot of possibilities. For example; querying an API to retrieve a list of options, or simply being able to build multiple similar options with a for loop. Most stores probably don't require anything like this however I have worked on a few stores where this would be beneficial.

The way I see this working is by using const jsonSchema =require(jsonSchemaFilePath) in line 22 of inject-schemas-to-sections.js. Since Node caches modules, they would need to be flushed out of the cache when updating, in order to be re-imported, but that's not a particularly difficult job.

@harshal317 harshal317 mentioned this pull request Feb 19, 2019
6 tasks
@t-kelly
Copy link
Contributor

t-kelly commented Feb 21, 2019

@davidwarrington copying your comment into #982. Please continue the conversation there!

@lock
Copy link

lock bot commented Mar 23, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Mar 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants