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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CodeSandbox CI to our PR workflow #6565

Merged
merged 2 commits into from Jan 17, 2020
Merged

Conversation

@wojciechczerniak
Copy link
Member

wojciechczerniak commented Dec 13, 2019

Context

CodeSandbox CI bot should follow the link to the issue, get the example and build it with newer HoT version. It should also take three examples from the config file and create new forks.

And as a developer I attach another example: https://codesandbox.io/s/amazing-cohen-9elvv
which should be included as well. Let's see 馃憖

How has this been tested?

It is a test 馃槃

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature or improvement (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Additional language file or change to the existing one (translations)

Related issue(s):

  1. #6563

Checklist:

  • My code follows the code style of this project,
  • My change requires a change to the documentation.
@codesandbox

This comment has been minimized.

Copy link

codesandbox bot commented Dec 13, 2019

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 051bd14:

Sandbox Source
vibrant-sun-y9vii Configuration
zealous-booth-p0htf Configuration
strange-banach-gwnjw Configuration
throbbing-water-qjqnr PR
@wojciechczerniak

This comment has been minimized.

Copy link
Member Author

wojciechczerniak commented Dec 13, 2019

My investigation showed that internal CodeSandbox requests resource with version missing in the URL to JSDelivr CDN:

Screenshot 2019-12-13 at 16 06 14

Might be something wrong in our build or CodeSandbox parser. Reported: codesandbox/codesandbox-client#3203

@wojciechczerniak wojciechczerniak self-assigned this Dec 27, 2019
@wojciechczerniak

This comment has been minimized.

Copy link
Member Author

wojciechczerniak commented Dec 27, 2019

I think I've found the issue. CodeSandbox has a problem with loading voc library. It cannot parse version to load it from CDN. voc is a dependency of bassel which is used by @handsontable/formulajs.

In earlier versions of bassel the voc version was not specified:

https://github.com/SheetJS/bessel/blob/31237637de54abece1ebf05944b3859c677b4c3b/package.json#L14

In that case, CodeSandbox should omit @ and load the latest version. That what the author intended. But it doesn't work like that ATM.

The current bassel version has voc version provided. It was even moved to devDeps so it doesn't have to be loaded by Handsontable either.

https://github.com/SheetJS/bessel/blob/2e2331329891dc5c16bda3461db0b9fe590ff2e9/package.json#L12

To move forward with this PR and don't wait for CodeSandbox (this is an edge case so they can ignore it for a long time) and I'm not sure if that will solve the issue (let's say we load latest vox, will it work with current formula.js ?) we can update the deps for formula.js to a newer bassel package:

https://github.com/handsontable/formula.js/blob/ae8f338f10f49dafe2b17cfa210202ceeb55f62b/package.json#L25

@budnix Do you think it will work with a simple package update for formula.js?

Maybe we could update the bassel and move back to jstat. They made some progress since we did a fork to fix the eval issue.

@wojciechczerniak

This comment has been minimized.

Copy link
Member Author

wojciechczerniak commented Jan 13, 2020

It's alive! Thx @budnix

WDYT @handsontable/devs @warpech ?

@warpech

This comment has been minimized.

Copy link
Member

warpech commented Jan 15, 2020

Amazing!

However, I have a few questions already:

  1. It does not load for me in Firefox (normal and incognito). Am I alone with that problem?
  2. It takes 15 seconds to load in Chrome and 25 seconds to load in Edge. This is quite annoying, can anything be done to speed this up?
  3. In CodeSandbox UI, can I force building any branch name or commit SHA of Handsontable and/or wrappers?
  4. If my PR has 50 commits, will CodeSandbox create 50 comments with different sandboxes? I am not saying it is bad, just asking :D I know that redundant comments can be hidden using GitHub UI.
  5. Why are there two slightly different sandboxes for React? https://codesandbox.io/s/vibrant-dew-d8hes, https://codesandbox.io/s/brave-sky-qobx8
  6. I can see that the React and Vue sandboxes are based on the source sandboxes, but what is the source for the vanilla sandbox (https://codesandbox.io/s/focused-silence-pq0ed)?
  7. Why are the first 3 sandboxes (for vanilla, Vue, React) called "Configuration" but the last one (for React, again) is called "PR")? In the issue
  8. Is there a possibility to give better names to the 4 sandboxes than "Configuration", "Configuration", "Configuration", "PR"? It is hard to find the source sandboxes for the new sandboxes, especially when there is a few of them.
  9. In CodeSandbox UI I can see that there is a tab called "Tests". Could we run our test suite there?
  10. Are the builds made by CodeSandbox usable outside of CodeSandbox, for example can I hot link them in my localhost?
@wojciechczerniak

This comment has been minimized.

Copy link
Member Author

wojciechczerniak commented Jan 15, 2020

  1. It does not load for me in Firefox (normal and incognito). Am I alone with that problem?

You're never alone! But it works for me. FF 72.0.1, OS X 10.15.

  1. It takes 15 seconds to load in Chrome and 25 seconds to load in Edge. This is quite annoying, can anything be done to speed this up?

15 sec on Chrome and Firefox for the first run. Then 3 sec witch cached sandboxes. I'm affraid it's up to them and I think they're doing quite well. Remember that everything has to be download from npm and build with webpack.

IMO It's still faster than recreating every use case example in JSFiddle with each new build. And would help @aninde and the reviewers a lot.

  1. In CodeSandbox UI, can I force building any branch name or commit SHA of Handsontable and/or wrappers?

I'm not aware of such a feature. CodeSandbox UI supports already build and published packages. But since CodeSandbox CI already does it for each commit it should be possible with the instructions in an answer to your 10th question.

  1. If my PR has 50 commits, will CodeSandbox create 50 comments with different sandboxes? I am not saying it is bad, just asking :D I know that redundant comments can be hidden using GitHub UI.

No. It will add only one comment and then update it with new links. Check the comment history for details: #6565 (comment) wrong link: #6564 (comment)

  1. Why are there two slightly different sandboxes for React? https://codesandbox.io/s/vibrant-dew-d8hes, https://codesandbox.io/s/brave-sky-qobx8

One is taken from the configuration (see files in this PR) and the second one is taken from the PR comment. I was lazy while creating this PR and made a copy&paste example with a small change. They should be quite different in a real-life use case.

  1. I can see that the React and Vue sandboxes are based on the source sandboxes, but what is the source for the vanilla sandbox (https://codesandbox.io/s/focused-silence-pq0ed)?

Sources for "Configuration" sandboxes are taken from this repo: https://github.com/handsontable/examples and are configured in .codesandbox/ci.json file in this PR: https://github.com/handsontable/handsontable/pull/6565/files#diff-3a497e46516b2c9e330c20af04452b77

  1. Why are the first 3 sandboxes (for vanilla, Vue, React) called "Configuration" but the last one (for React, again) is called "PR")? In the issue

"Configuration" sandboxes are taken from the .codesandbox/ci.json file and are always run with the CI. Their purpose is to check some generic use cases with each PR, ie. wrappers.

"PR" sandboxes are the ones that bot found in the PR. They are PR specific. Those could be use cases that the developer used to implement/demonstrate the feature or bug fix.

One type is missing in this PR and it's called "Issue". Those are the sandboxes that we're found by the bot in the issue that was mentioned in the PR comment. If there we're a use case scenarios reported there by the user, quality assurance or project manager (among others) within the issue we're fixing in the PR we will be able to verify that the original use case scenario was fixed as well.

  1. Is there a possibility to give better names to the 4 sandboxes than "Configuration", "Configuration", "Configuration", "PR"? It is hard to find the source sandboxes for the new sandboxes, especially when there is a few of them.

Unfortunately no. It might be a good feature request.

  1. In CodeSandbox UI I can see that there is a tab called "Tests". Could we run our test suite there?

We would have to add tests to sandboxes in https://github.com/handsontable/examples. It's more for adding additional tests for a particular example than running our tests suite again. The tests we're already run with Codeship CI.

  1. Are the builds made by CodeSandbox usable outside of CodeSandbox, for example can I hot link them in my localhost?

Yes. Instructions are available here: https://ci.codesandbox.io/status/handsontable/handsontable/pr/6565/builds/4801

@aninde

This comment has been minimized.

Copy link

aninde commented Jan 16, 2020

My proposal of advanced, but also universal settings:

const settings = {
  colHeaders: true,
  rowHeaders: true,
  width: 600,
  height: 500,
  contextMenu: true,
  manualColumnFreeze: true,
  fixedRowsTop: 3,
  fixedColumnsLeft: 3,
  fixedRowsBottom: 3,
  manualColumnMove: true,
  manualColumnResize: true,
  manualRowResize: true,
  manualRomMove: true,
  dropdownMenu: true,
  filters: true,
  columnSorting: true,
  comments: true,
  licenseKey: 'non-commercial-and-evaluation',
};
@AMBudnik

This comment has been minimized.

Copy link
Contributor

AMBudnik commented Jan 16, 2020

I would also turn on comments they are one of those things people use pretty regularly in Excel.

@wojciechczerniak wojciechczerniak requested a review from budnix Jan 17, 2020
@budnix
budnix approved these changes Jan 17, 2020
Copy link
Member

budnix left a comment

馃

@wojciechczerniak wojciechczerniak merged commit ba842f5 into develop Jan 17, 2020
3 checks passed
3 checks passed
ci/codesandbox Building packages succeeded.
Details
continuous-integration/codeship Build succeeded
Details
security/snyk - package.json (krzysztofspilka) No new issues
Details
@wojciechczerniak wojciechczerniak deleted the feature/issue-6564 branch Jan 17, 2020
jansiegel added a commit that referenced this pull request Feb 3, 2020
* Create CodeSandbox CI configuration file
* Use sandboxes created for testing pull requests instead basic ones
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can鈥檛 perform that action at this time.