Skip to content
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

Open
wants to merge 50 commits into
base: master
Choose a base branch
from

Conversation

lordofthelake
Copy link
Contributor

@lordofthelake lordofthelake commented Jan 20, 2020

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

Major changes

  • Images now get fetched and displayed without any re-encoding, re-compression, or other manipulations.

Other changes

  • Travis CI now runs the test suite on both Node.js 12.x and 14.x. The npm cache is temporarily disabled, because it causes issues with npm 7. It can probably be re-enabled if someone with admin access to Travis purges all the caches from their UI.

Many of the changes that were initially part of this PR have already been added by @mathieudutour in separate PR's.

# Breaking changes
* render and renderToJSON 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 as import * as ReactSketchapp from 'react-sketchapp'
* There are different entrypoints for each platform. It needs a somewhat recent version of skpm to bundle correctly, one that reads the sketch field in package.json.
* Platform.Version now returns the actual Sketch version the plugin is running in, as a string, instead of being fixed to be 1. 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, a PlatformBridge. 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 running react-sketchapp in the browser (creating the appropriate bindings).

## Internal changes
* Platform-specific modules now use dynamic imports or double file extensions instead of using evals 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.

@lordofthelake lordofthelake force-pushed the pure-make-image-from-url-implementation branch from f9952f5 to 2968a64 Compare January 20, 2020 16:53
@lordofthelake lordofthelake force-pushed the pure-make-image-from-url-implementation branch from 3aab5da to dfd88c9 Compare January 21, 2020 13:32
@lordofthelake lordofthelake force-pushed the pure-make-image-from-url-implementation branch from 9e0744f to 76e4899 Compare January 22, 2020 16:27
@lordofthelake lordofthelake force-pushed the pure-make-image-from-url-implementation branch from 29db20d to b267fcd Compare January 22, 2020 19:48
@lordofthelake
Copy link
Contributor Author

@mathieudutour Before I spend time debugging it, is npm run test:e2e supposed to work? I see it's commented out on .travis.yml and it's erroring out in a cryptic way for me, so before investing the time I wanted to figure out whether it's me or it was broken before.

npm run test:e2e

> [email protected] 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'
}

@lordofthelake
Copy link
Contributor Author

@mathieudutour Friendly ping :)
I would appreciate some feedback on whether the overall direction is to incorporate this change at all or not, since we have a bunch of work downstream influenced by this.

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.

@mathieudutour
Copy link
Collaborator

Sorry for the delay. Yeah, happy with how it's going 👍

@macintoshhelper
Copy link
Contributor

macintoshhelper commented May 10, 2020

@mathieudutour Before I spend time debugging it, is npm run test:e2e supposed to work? I see it's commented out on .travis.yml and it's erroring out in a cryptic way for me, so before investing the time I wanted to figure out whether it's me or it was broken before.

npm run test:e2e

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.

It appears to be a problem on Sketch's side with sketchtool, or with skpm-test. Importing the test plugin manually (open node_modules/@skpm/test-runner/, double clicking test-runner.sketchplugin and using sketch-dev-tools to see console logs works as a hacky fix for now.

Running npm run test:e2e in git checkout v3.0.0-beta3 should help figure out if this is an issue introduced by a Sketch update or not (e2e testing worked in that commit/git snapshot).

As mentioned in #346 , the plugin is required to export commonjs modules.

@macintoshhelper
Copy link
Contributor

macintoshhelper commented Sep 13, 2020

@lordofthelake Hi, btw, I found what was breaking the e2e tests. Adding this to the package.json should fix it (downgrading @skpm/test-runner to 0.3.x from 0.4.x):

{
// ...
  "dependencies": {
// ...
    "@skpm/test-runner": "^0.3.2",
  }

Ref: #516

@lordofthelake lordofthelake marked this pull request as draft September 13, 2020 21:11
@lordofthelake lordofthelake changed the title New API, platform bridges and "pure" image fetching Async rendering API and "pure" image fetching Jan 6, 2021
@lordofthelake lordofthelake marked this pull request as ready for review January 6, 2021 19:28
@lordofthelake
Copy link
Contributor Author

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 Promises is not retro-compatible

@lordofthelake
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants