-
Notifications
You must be signed in to change notification settings - Fork 821
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
e2e tests #346
e2e tests #346
Conversation
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.
A few changes looped in here that I am not sure are necessary.
Additionally, is anything stopping us from running e2e tests on Travis? https://github.com/skpm/skpm/tree/master/packages/test-runner#running-the-tests-on-travisci
"presets": ["module:@skpm/babel-preset", "@babel/preset-flow"], | ||
"plugins": ["@babel/plugin-proposal-class-properties", ["@babel/plugin-transform-modules-commonjs", { | ||
"noInterop": true | ||
}]] |
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.
Is this required by skpm?
["@babel/plugin-transform-modules-commonjs", { "noInterop": true }]
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.
The plugin is required to export commonjs modules. It’s not the default with Babel 7 anymore. I chose to add the noInterop option because it reduces the bundle size a lot. But it also means that I needed to change a bunch of imports in order to make them compatible. For example react
doesn’t export a default
so you need to import * as react
from it. This is actually the correct way to import a commonjs package (typescript enforces it).
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 had no idea that React doesn't ship an es module distro 🙀
I suppose this all makes sense internally.
Externally, should we be updating our docs and examples? While it may be correct, wonder if it may cause confusion. And maybe I am incorrect here, but I would expect most people are used to seeing import React from 'react'
.
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.
yeah happy to change the docs back
@@ -37,7 +37,7 @@ Managing the assets of design systems in Sketch is complex, error-prone and time | |||
## What does the code look like? | |||
|
|||
```js | |||
import React from 'react'; | |||
import * as React from 'react'; |
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.
What is the reasoning for altering all of these imports?
|
||
const page = sketch.Page.fromNative(nativePage); | ||
expect(page.layers[0].name).toBe('Swatches'); | ||
}); |
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 an absolutely awesome start 🙌
Nothing stopping us from running it on Travis no, just didn’t have time to set it up yet. |
I'd love to see this run on CI. Can be done in a standalone PR if you prefer... |
To run the E2E tests:
npm run test:e2e
. That will start sketch and run the tests which are under__tests__/skpm/
There might be some changes in the generated files due to updating to babel 7, I haven't checked yet