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
Feat/forestry migration updates #3510
Conversation
🦋 Changeset detectedLatest commit: d9d27a4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Modified PackagesThe following packages were modified by this pull request:
|
public addErrorName(error: NameError) { | ||
this.allErrorNames.push(error) | ||
} | ||
public printErrorNames() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if I get what's happening in this function. It looks like we'd log out several different error types when called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only for "name errors".
For example if you had a template name "my-template" or a field name "my-field".
|
||
const errorSingletonInstance = ErrorSingleton.getInstance() | ||
|
||
export const stringifyName = (name: string, template: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we changing the name of their fields in their content pages along with this? I have a bit of concern it might be some overhead for the user to update their site to make sure it doesn't break the build (as opposed to, if we were to just support characters like dashes in our field names)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jamespohalloran No we are not updating the users content. This would require recursively traversing all of their files and the new config at the same time and keeping track of where you are in the tree, the old name and the new name. I could see this being a rabbit hole and costing 1-2 weeks of work to get it right. I left it out of this scope of work on purpose for this reason.
The user will also have to update there code as well (we definitely cant migrate their code).
packages/@tinacms/cli/src/cmds/forestry-migrate/util/errorSingleton.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one small comment on the naming of a function.
Overall this looks great. One thing though, I think we might be leaving the user in a bit of a confusing state by renaming the fields. Out of the box, the fields will look like they are working, however when editing they'll just update a new field that isn't used by their site.
We can address this in a next step, but I think supporting dashes in field names may be worth the investment.
This PR updates the forestry migration tool in a couple of ways.
_tempalte
key to documentsSteps to test.
Run
and then run
I am going to work on some docs that we can link to for certain errors that occur.