Shopify / slate

Slate is a toolkit for developing Shopify themes. It's designed to assist your workflow and speed up the process of developing, testing, and deploying themes.
https://shopify.github.io/slate
MIT License
1.28k stars 364 forks source link

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

Closed MikhailRoot closed 5 years ago

MikhailRoot commented 5 years ago

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.

https://github.com/Shopify/slate/issues/749 https://github.com/Shopify/slate/issues/856

Checklist

For contributors:

For maintainers:

t-kelly commented 5 years ago

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

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

harshal317 commented 5 years ago

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

Hehe sorry typos were bugging me

MikhailRoot commented 5 years ago

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

harshal317 commented 5 years ago

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 commented 5 years ago

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

MikhailRoot commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

Sorry, closed PR by accident. Reopened it :)

t-kelly commented 5 years ago

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

MikhailRoot commented 5 years ago

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

t-kelly commented 5 years ago

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!

davidwarrington commented 5 years ago

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.

t-kelly commented 5 years ago

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

lock[bot] commented 5 years ago

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.