-
Notifications
You must be signed in to change notification settings - Fork 819
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
Async rendering API and "pure" image fetching #472
base: master
Are you sure you want to change the base?
Async rendering API and "pure" image fetching #472
Conversation
f9952f5
to
2968a64
Compare
3aab5da
to
dfd88c9
Compare
9e0744f
to
76e4899
Compare
29db20d
to
b267fcd
Compare
@mathieudutour Before I spend time debugging it, is npm run test:e2e
> react-sketchapp@3.0.5 test:e2e /Users/michelepiccirillo/Development/COMPOSITIVE/react-sketchapp
> skpm-test
Looking for the test files
RUNS basic
RUNS render-context
RUNS render-in-wrapped-object
Building the test plugin source
Compiling the test plugin
Running the tests
error Error while running the tests after build
Error: Command failed: "/Applications/Sketch.app/Contents/Resources/sketchtool/bin/sketchtool" run "/Users/michelepiccirillo/Development/COMPOSITIVE/react-sketchapp/node_modules/@skpm/test-runner/test-runner.sketchplugin" "plugin-tests" --without-activating
at ChildProcess.exithandler (child_process.js:295:12)
at ChildProcess.emit (events.js:210:5)
at maybeClose (internal/child_process.js:1021:16)
at Socket.<anonymous> (internal/child_process.js:430:11)
at Socket.emit (events.js:210:5)
at Pipe.<anonymous> (net.js:659:12) {
killed: false,
code: 1,
signal: null,
cmd: '"/Applications/Sketch.app/Contents/Resources/sketchtool/bin/sketchtool" run "/Users/michelepiccirillo/Development/COMPOSITIVE/react-sketchapp/node_modules/@skpm/test-runner/test-runner.sketchplugin" "plugin-tests" --without-activating'
} |
@mathieudutour Friendly ping :) If affirmative, I will do the last round of polishing with docs & tests so that this can go in properly; otherwise, I'll just move on with a different approach for our code. |
Sorry for the delay. Yeah, happy with how it's going 👍 |
Hi @lordofthelake , just noticed this comment. I actually ran into the same issue on #516 . I'm working on fixing up the e2e tests, and adding e2e tests for all of the examples, with the view of integrating visual regression testing later on.
As mentioned in #346 , the plugin is required to export commonjs modules. |
@lordofthelake Hi, btw, I found what was breaking the e2e tests. Adding this to the {
// ...
"dependencies": {
// ...
"@skpm/test-runner": "^0.3.2",
} Ref: #516 |
This has been around for a while, although 50%+ of the changes proposed here have already been incorporated. I have cleaned up the rest of the changes and resolved the merge conflicts. There should probably be a major version bump with this, since the switch to returning |
The build is red for what I suspect is a dirty npm cache (nodejs/help#2874). I would try to clean Travis' cache, but I don't have enough permissions to do so. Maybe @mathieudutour can help with this. |
…compositive/react-sketchapp into pure-make-image-from-url-implementation
This opens the way to adding a pure Node.js bridge, that can be run on different platforms. It also improves significantly the rendering performance in image-heavy layouts, as the rendering doesn't stop on network requests (in an informal test by @macintoshhelper, the improvement can be as much as 2x).
Breaking changes
render
,renderToJSON
andmakeSymbol
now are async methods, returning Promises (Proposal: makingrender()
async #469)Major changes
Other changes
Many of the changes that were initially part of this PR have already been added by @mathieudutour in separate PR's.
# Breaking changes*render
andrenderToJSON
are completely split.render
is meant to be used exclusively while embedded in Sketch.renderToJSON
can be used in either scenario.* There isn't a default export anymore.import ReactSketchapp from 'react-sketchapp'
must now be rewritten asimport * as ReactSketchapp from 'react-sketchapp'
* There are different entrypoints for each platform. It needs a somewhat recent version ofskpm
to bundle correctly, one that reads thesketch
field inpackage.json
.*Platform.Version
now returns the actual Sketch version the plugin is running in, as a string, instead of being fixed to be1
. If it's running under Node or another environment, it returns an empty string.## Major changes*render
,renderToJSON
,makeSymbol
,TextStyles.registerStyle
,TextStyles.create
now take an optional extra parameter, aPlatformBridge
. It is already initialized smartly for each platform, but can be overridden by the user. This opens the possibility for adding other bridges (addressing, for example, #465) or even runningreact-sketchapp
in the browser (creating the appropriate bindings).## Internal changes* Platform-specific modules now use dynamic imports or double file extensions instead of usingevals
to throw webpack off the scent.* Different implementations of the same methods are now grouped by method, instead of being split in different directories per platform.