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

JSON Object Archives #306

Merged
merged 19 commits into from Dec 20, 2018

Conversation

Projects
None yet
3 participants
@WCByrne
Copy link
Contributor

WCByrne commented Dec 5, 2018

This is a basic implementation of #292 and allows:

  • Fetching a dictionary archive of an object
  • Restoring an object from an archive dictionary

Not sure if this is the best way to archive/unarchive objects but it has been working for me in my plugin. Also very open to naming suggestions.

@mathieudutour
Copy link
Contributor

mathieudutour left a comment

So I still have mixed feeling about this.

It feels weird to have a method on wrappedObject `to export and one on the root object to import.

How about, instead of the method on wrappedObject, we change the export method to handle an additional format: json? It would then return the JSON archive. That way it’s pretty clear that you are exporting the layer to another format. And it would make more sense to have fromSketchJSON on the root object.

Sorry to be annoying but we need to be careful before introducing an API as it’s really hard to remove it

Show resolved Hide resolved Source/dom/WrappedObject.js Outdated
Show resolved Hide resolved Source/dom/__tests__/Archive.test.js Outdated
Show resolved Hide resolved Source/dom/wrapNativeObject.js Outdated
Show resolved Hide resolved Source/dom/WrappedObject.js Outdated
Show resolved Hide resolved Source/dom/index.js Outdated
@WCByrne

This comment has been minimized.

Copy link
Contributor Author

WCByrne commented Dec 6, 2018

I like the idea of considering this an export 💯 . I was thinking there was a better place for this to live but went with the easier option to start.

@WCByrne

This comment has been minimized.

Copy link
Contributor Author

WCByrne commented Dec 6, 2018

@mathieudutour Since the exportObject saves exported data to a path, do you think it makes sense to do incorporate this export into the same method? Especially considering the output is a list of desired types.

Thinking we could...

  1. If the output === json or contains json, return that immediately (i.e. you can't have multiple types with json)
  2. Make a new export method next to exportObject
  3. Save the JSON - I don't think this is a good option especially considering the other formats happen inside the native exporter.

Option 1 seems like the cleanest to me.

@mathieudutour

This comment has been minimized.

Copy link
Contributor

mathieudutour commented Dec 6, 2018

Ah yes I should have explained a bit further. There is this other issue that I want to work on as well:
#185.

I was thinking of the following:

  • if output is falsy, then return the data instead of writing it to disk.
  • if there is an output, then write the json string to ${output}/export.json

I realised that there will be a disparity in the API until #185 is done but we are building for the future :)

@WCByrne

This comment has been minimized.

Copy link
Contributor Author

WCByrne commented Dec 6, 2018

Sorry I meant format equals or contains json.

Is there any concern in changing the behavior of a falsey output? Right now it defaults to the ~/Documents/Sketch Exports right?

As for implementing, is there a way to get the data back from the exporter being used or does that need to be handled differently? I use MSExportRequest in my plugin... 🤔

Also, thoughts on returning in the case of !option and the fact that formats can be an array? Would that be considered invalid? Return the first one?

@mathieudutour

This comment has been minimized.

Copy link
Contributor

mathieudutour commented Dec 6, 2018

  • If format is json, then only export use the new code path

  • if format contain json, then we remove it from the format string, pass it to the old code path and use the new code path as well

  • if output is falsy (it will default to a path but can still be overwritten by a falsy value), then we return the value (only for the json for now, the implementation for the images will be in another PR)

  • if it’s a path, then we write to a file

WCByrne added some commits Dec 6, 2018

Rename fromSketchJSON
Fix export json file saving
Show resolved Hide resolved Source/dom/export.js Outdated
@mathieudutour
Copy link
Contributor

mathieudutour left a comment

Nearly there

Show resolved Hide resolved Source/dom/export.js Outdated
Show resolved Hide resolved Source/dom/export.js Outdated
Show resolved Hide resolved Source/dom/export.js Outdated
Show resolved Hide resolved Source/dom/export.js Outdated
Show resolved Hide resolved Source/dom/export.js Outdated
Show resolved Hide resolved Source/dom/export.js Outdated
Apply suggestions from code review
Co-Authored-By: WCByrne <wesburn@me.com>
@WCByrne

This comment has been minimized.

Copy link
Contributor Author

WCByrne commented Dec 10, 2018

Am I missing something about running tests? with npm run test I get Xcode build not found. With npm run test:no-variant I get Error: couldn't find the test results.

I can see the tests running in Sketch and saving things to my Desktop 🤔

@mathieudutour

This comment has been minimized.

Copy link
Contributor

mathieudutour commented Dec 10, 2018

Hum not sure. You can non run test:no-variant IIRC

@WCByrne

This comment has been minimized.

Copy link
Contributor Author

WCByrne commented Dec 10, 2018

That's what gives Error: couldn't find the test results. Same as skpm/skpm#191

WCByrne added some commits Dec 10, 2018

Ensure png default explicitly
send string format list to exporter

Add export tests
export cleanup and logic organization
Document objectFromJSON
@mathieudutour

This comment has been minimized.

Copy link
Contributor

mathieudutour commented Dec 14, 2018

(I'm not forgetting this, I'll set time aside to test it properly next week)

@mathieudutour

This comment has been minimized.

Copy link
Contributor

mathieudutour commented Dec 17, 2018

I opened a PR on your repo to refactor a bit: WCByrne@b6c3343

Feel free to review it and merge it and then we can merge this one

WCByrne added some commits Dec 18, 2018

@WCByrne

This comment has been minimized.

Copy link
Contributor Author

WCByrne commented Dec 18, 2018

Thanks for cleaning that all up. I removed what like there was some leftover exporting happening at the end of the main export function. This PR should have the latest now.

@WCByrne

This comment has been minimized.

Copy link
Contributor Author

WCByrne commented Dec 18, 2018

Heads up. Assuming the betas pickup the local dev API from defaults, this doesn't seem to be working in 53 😕

Duh, had to set the api path under a different defaults name. All is good.

@mathieudutour mathieudutour merged commit cc85c56 into BohemianCoding:develop Dec 20, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
deploy/netlify Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment