[Scripts] Support using script.config.yml file for script configuration

[adampetro]

WHY are these changes introduced?

Closes https://github.com/Shopify/script-service/issues/3729

As explained in the issue, we want to more closely align with other teams like UI Extensions.

WHAT is this pull request doing?

Adds support for using a script.config.yml file for encoding Script configuration. If the script.config.yml is not present but a script.json is, we will use that.

How to test your changes?

See that script.json still works

1. Create a new script with shopify script create and select shipping-methods API and TypeScript language
2. See that script.json has the right title (what you entered as the name of the script)
3. Update the script.json to have some fields
4. Push the script with shopify script push
5. See the configuration fields when trying to enable the customization on your shop in the shipping settings

See that script.config.json works

1. Create a new script with shopify script create --branch=ap/script-config-yml and select shipping-methods API and TypeScript language
2. See that the script.config.yml has the right title (what you entered as the name of the script)
3. Update the script.config.yml to have some fields
4. Push the script with shopify script push
5. See the configuration fields when trying to enable the customization on your shop in the shipping settings

See that no configuration file raises error

1. Remove the script.json or script.config.yml from one of the above script projects you created
2. Try to push the script with shopify script push
3. See an error message that a script.config.yml file must be present

See that removing required fields raises error

1. Remove the title or version field from the script.json or script.config.yml from one of the above script projects you created
2. Try to push the script with shopify script push
3. See an error message that the configuration file is missing the field you removed

Update checklist

• I've added a CHANGELOG entry for this PR (if the change is public-facing)
• I've considered possible cross-platform impacts (Mac, Linux, Windows).
• I've left the version number as is (we'll handle incrementing this when releasing).
• I've included any post-release steps in the section above.

[TR1GG3RElF]

Read

• CHA NGELOG.md

[jacobsteves]

looks pretty good overall!

[ErinRenNumber1]

Just took a very quick look, and I think some of the code is just temporary, so, please don't feel obligated to change anything. I do think the relationship is not that ScriptConfigRepo has ScriptJsonRepo and ScriptConfigYmlRepo. I think it's really ScriptJsonRepo/ScriptConfigYmlRepo is a ScriptConfigRepo. So, I think the inheritance relationship would be better here, with consistent contract in ScriptConfigRepo as the parent.

[adampetro]

The reason that I didn't use inheritance was because the logic in the ScriptConfigRepo uses both the JSON and YML repo in its instance methods, and I don't think it is good practice for a base class to know about the subclasses in instance methods. If we did want to use inheritance, I suppose we could move the logic that is currently in ScriptConfigRepository's instance methods to a separate class or perhaps instance methods on ScriptProjectRepository. WDYT?

[jacobsteves]

If we wanted to keep this logic long term, then I would agree that inheritance makes more sense here. Since we have a stretch goal to remove support for script.json next cycle, I personally think what we have here would be fine for now. But I can give my thoughts if we want to bikeshed in the meantime 😄

I don't think it is good practice for a base class to know about the subclasses in instance methods

+1 I definitely agree! I think both your alternative ideas could work. Since the ScriptProject is the aggregate root over the script config files, I think it would make sense to leave logic in the ScriptProjectRepo as long as it isn't too noisy (which i don't think it would be since its rather minimal).

[andrewhassan]

Yeah, I agree that it makes more sense that the JSON and YAML repositories are subclasses of the ScriptConfigRepository. Doing this means that we can isolate the common logic in the parent class and have the subclasses supply their own customizations (e.g. file name and parsing).

[adampetro]

I updated this to use inheritance. I personally don't think it adds much value in this case and found it to make the code more complex as there isn't that much in common between the two subclasses. Let me know if you think the change is worthwhile, otherwise I can revert the commit.

[jacobsteves]

There isn't that much in common between the two subclasses

I think you can actually find a lot more in common, depending on how you think of it! I see the ScriptConfigYmlRepo and the ScriptJsonRepo differing only by how they parse file contents and write file contents. So in the base class we can abstract all the functionality to do with getting, setting, and creating ScriptConfigs and we could write some template methods in the subclasses that handle understanding file contents. I did some thinking and I threw together a mock of that:

Code dropdown because its kinda long

# in ScriptProjectRepository
def script_config
  script_config_repo.get || raise("No config file")
End

def script_config_repo
  supported_repos = [ScriptConfigYmlRepository.new, ScriptJsonRepository.new]
  supported_repos.find { |repo| repo.active? }
end

class ScriptConfigRepo
  def active?
    ctx.file_exist?(filename)
  end

  def get
    return nil unless active?
    from_h(file_content_to_hash(ctx.read(filename)))
  end

  def set
    hash = get&.content || {}
    hash[“version”] = version
    hash[“title”] = title

    ctx.write(filename, hash_to_file_content(hash))

    from_h(hash)
  end

  # subclasses must override
  def version; end
  def filename; end
  def file_content_to_hash(hash); end
end

class ScriptConfigYmlRepo < ScriptConfigRepo
  def version
    2
  end

  def hash_to_file_content(hash)
    YAML.dump(hash)
  end

  def file_content_to_hash(content)
    begin
      hash = YAML.load(content)
    rescue Psych::SyntaxError
      raise Errors::InvalidScriptConfigYmlDefinitionError
    else
      raise Errors::InvalidScriptConfigYmlDefinitionError unless hash.is_a?(Hash)
      hash
    end
  end
end

class ScriptJsonRepo < ScriptConfigRepo
  def version
    1
  end

  def hash_to_file_content(hash)
    JSON.pretty_generate(hash)
  end

  def file_content_to_hash(content)
    JSON.parse(content)
  rescue JSON::ParserError
    raise Errors::InvalidScriptJsonDefinitionError
  end
end

Now I haven't ran that code so don't take my word that it works. And also I definitely don't want to force you to use code that I wrote by myself so please don't think you have to use it or change what you have 😆 But that could be something that works if we wanted to keep this longterm. But then again, it isn't as flexible if we want to support different structures of the file 🤷‍♂️

Like I mentioned before, I don't think this really matters because its temporary and could be removed next cycle. I'm good with whatever you think is best in the meantime!

[adampetro]

Thanks for the detailed answer! I applied a lot of your suggestion to the get method, which is now only implemented in the base class. I think the updating is more nuanced with the ScriptConfigYmlRepository having an update_or_create method and the ScriptJsonRepository only having an update method so I did not use inheritance on those methods.

[andrewhassan]

Yeah, I agree that it makes more sense that the JSON and YAML repositories are subclasses of the ScriptConfigRepository. Doing this means that we can isolate the common logic in the parent class and have the subclasses supply their own customizations (e.g. file name and parsing).

[andrewhassan]

Thanks for making those changes! Overall, I think the approach makes sense. My main comment is basically what Jacob said; if we're going with an inheritance approach to the ScriptConfigRepository, I think more logic could be shared between the classes.

[andrewhassan]

Nit: It might be better practice to raise NotImplementedError here.

[adampetro]

Reading the docs on NotImplementedError, I don't think this is an appropriate use as it is not because the platform/OS does not support what we are trying to do.

[andrewhassan]

My bad, I didn't realize that NotImplementedError already existed 🤦

This class is effectively an abstract class, and these methods are abstract methods. Since Ruby doesn't have the concept of abstract methods, one of the suggestions in POODR was to raise an error for methods that should be implemented by subclasses. The reason is that a future developer may miss that their subclass needs to implement some required method, which could cause a failure downstream (e.g. file_content_to_hash is not implemented so the contents are nil, which cause an error somewhere else in the program).

[adampetro]

We do use NotImplementedError for this in purpose in this codebase so maybe its fine

[andrewhassan]

In POODR, they don't mention the implementation of NotImplementedError; they only raise it so I'm assuming it's that error.

[andrewhassan]

I'd consider this a nit though, so feel free to make changes or not 😛

[andrewhassan]

LGTM!

[jacobsteves]

I tried tophatting your section on See that script.config.yml works and the script title wasn't updated to the title I entered upon creation. Other than that, the tophat worked well

[jacobsteves]

Sorry ignore me, upon trying again it works? I probably messed something up the first time 😄 Looks great!! 🎉