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 sync utils #1171

Merged
merged 4 commits into from Mar 18, 2020
Merged

Add sync utils #1171

merged 4 commits into from Mar 18, 2020

Conversation

@andyrichardson
Copy link
Contributor

andyrichardson commented Mar 16, 2020

About

Fix react-cosmos/rfcs#13 (comment)

Notes

Haven't tested this yet - looking to see if this is the way we want to go about adding sync support 馃憤

@skidding

This comment has been minimized.

Copy link
Member

skidding commented Mar 16, 2020

@andyrichardson Thanks for looking into this!

To stay simple, my approach would be to make every (relevant) internal function sync, because RC is a dev tool and I don't think IO concurrency is a performance concern. I think the only reason I defaulted to async initially is because that's the best practice for production apps. Oh well, live and learn :).

So we could export separate sync/async functions[0] for the server-side API that call the same newly-sync internal functions. The reason for this would be to not break the current async API[1].

[0] Ancient programming wisdom says two methods are the better alternative to a boolean argument 馃榿.

[1] If everybody used async/await we could just make everything sync without breaking existing implementations, because you can await on a sync function 鈥 am I wrong? 馃But if the Promise API is called directly like getFixtures().then..., making existing functions sync would be a breaking change. So we can't just make everything sync for existing users.

The current async APIs would become mere Promise wrappers for the newly sync APIs.

export async function getFixtures() {
  return new Promise((resolve, reject) => {
    // Is this try/catch even needed? 馃
    try {
      resolve(getFixturesSync())
    } catch (err) {
      reject(err);
    }
  })
}

Obviously this would slightly change runtime behavior for existing users, but the API will be the same. I don't see how it would affect anyone, but can you think of any reason why this would be a problem?

@andyrichardson

This comment has been minimized.

Copy link
Contributor Author

andyrichardson commented Mar 16, 2020

@skidding sound like a good shout. I'll amend the PR tomorrow.

can you think of any reason why this would be a problem

In theory, it could mean the node process would be blocked while waiting for I/O reads - in practice, I don't think anyone is going to notice a difference.

@skidding

This comment has been minimized.

Copy link
Member

skidding commented Mar 16, 2020

@skidding sound like a good shout. I'll amend the PR tomorrow.

Great, thanks!

In theory, it could mean the node process would be blocked while waiting for I/O reads - in practice, I don't think anyone is going to notice a difference.

I think it means both in practice. Reading the config file and globbing the file system for fixture/decorator paths (note that the contents of user files are not read in the process) will have to be blocking for this PR to work. But I agree with the second point that it's unlikely for anyone to notice a difference.

I'm actually excited for this because I've also struggled in the past with generating one test per fixture, as opposed to loading all fixtures in a long async test, which I ended up doing.

@andyrichardson

This comment has been minimized.

Copy link
Contributor Author

andyrichardson commented Mar 17, 2020

I'm actually excited for this because I've also struggled in the past with generating one test per fixture, as opposed to loading all fixtures in a long async test, which I ended up doing.

Awesome to hear!

Just cleaned up the branch and now there's an async version of getFixtureUrls and getFixtures

@skidding

This comment has been minimized.

Copy link
Member

skidding commented Mar 18, 2020

Great stuff!

There's two places where we're still awaiting on no-longer-async functions:

Can you please remove those awaits?


PS. This is completely off topic, but maybe you know more about the TS compiler.
VS Code shows these useful diagnostics:

image

But they are not errors and they don't show up when calling tsc, so they are easy to miss. It would be great to get these diagnostics with a command, instead of having to open a file in VS Code to notice them. No worries if you don't know anything about this :).

@andyrichardson

This comment has been minimized.

Copy link
Contributor Author

andyrichardson commented Mar 18, 2020

Oh yeah sorry I missed that!

If you're using Babel to transpile typescript, last I heard they remove the types during transpilation so there's no actual type checking enforced.

In the past, I've added this to CI to enforce type safety

tsc --noEmit
@skidding

This comment has been minimized.

Copy link
Member

skidding commented Mar 18, 2020

You're right, Babel just strips out the types. So we have a separate command for type checking with tsc like you say. Awaiting on a non-promise response isn't actually a type error. The code works just fine, which is why the build is green. It's just some sort of extra nice-to-have diagnostics that only show up in VS Code :).

@andyrichardson

This comment has been minimized.

Copy link
Contributor Author

andyrichardson commented Mar 18, 2020

Just pushed those changes 馃憤

FYI I wasn't able to get dependencies installed without deleting the lockfile. One of the locked dependencies seems to be expecting node 9.

yarn install v1.17.3
[1/4] 馃攳  Resolving packages...
[2/4] 馃殮  Fetching packages...
error upath@1.0.4: The engine "node" is incompatible with this module. Expected version ">=4 <=9". Got "11.12.0"
error Found incompatible module.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

Rebuilding the lockfile got around this issue (I haven't included that in this PR)

};

module.exports = async function injectUserDeps(source: string) {
module.exports = function injectUserDeps(this: loader.LoaderContext) {

This comment has been minimized.

Copy link
@andyrichardson

andyrichardson Mar 18, 2020

Author Contributor

My first time having to do this (excuse the pun).

It's a pseudo parameter.

This comment has been minimized.

Copy link
@skidding

skidding Mar 18, 2020

Member

馃啋

@skidding

This comment has been minimized.

Copy link
Member

skidding commented Mar 18, 2020

Hmm. There was no need to make generateUserDepsFile and injectUserDeps sync. What I meant was literally just to remove the await keyboard in two specific lines: userDepsLoader.ts:30 and userDepsFile.tsx:54.

Sorry if I mislead you, I know this back and forth can be annoying 馃槄. Do you mind if you revert the last commit and just do that?

FYI I wasn't able to get dependencies installed without deleting the lockfile.

Yeah that thing annoys me. It's because of an old transient dependency of chokidar that I don't have control over. You go around it by installing dependencies like this:

yarn --ignore-engines
@andyrichardson

This comment has been minimized.

Copy link
Contributor Author

andyrichardson commented Mar 18, 2020

Oh my bad - the functions are synchronous and webpack supports synchronous loaders so I thought this would be a better option than running sync code and triggering the callback?

Yeah that thing annoys me

have you tried rebuilding the lockfile? It worked for me.

rm yarn.lock
yarn
git add yarn.lock
git commit -m "Rebuild lockfile"

I do this pretty frequently as yarn tries to stick to whatever is already in the lockfile - which can cause you not to get patch updates on dependencies. If you run yarn audit on the current lockfile you'll see what I mean.

Before

39687 vulnerabilities found - Packages audited: 1257513
Severity: 6640 Low | 7968 Moderate | 25077 High | 2 Critical
鉁  Done in 62.83s.

After rebuild

5 vulnerabilities found - Packages audited: 1259435
Severity: 5 Moderate
   Done in 3.09s.

IMHO yarn should be rebuilding the lock file every install unless --frozen-lockfile is specified otherwise you get this kind of security issue.

@skidding

This comment has been minimized.

Copy link
Member

skidding commented Mar 18, 2020

Oh my bad - the functions are synchronous and webpack supports synchronous loaders so I thought this would be a better option than running sync code and triggering the callback?

Fair enough, I agree there's no need to keep the webpack loader async if everything it does is sync.

But maybe we should keep generateUserDepsFile async because we can, and since writing the user deps file is done whenever the user saves a relevant source file, the performance gain of async IO might matter. What do you think?

Btw, the user deps file is only relevant for React Native users, as an alternative to the webpack loader.

have you tried rebuilding the lockfile? It worked for me.

I haven't. Let's try this in another PR because it seems promising.

IMHO yarn should be rebuilding the lock file every install unless --frozen-lockfile is specified otherwise you get this kind of security issue.

Not sure about this. The benefit of the lock file is to pin down versions of direct and transient dependencies to have more predictability. Otherwise a dependency of a dependency can introduce a bug in patch/minor version and you'll get that bug whenever you install something unrelated. But I'm for updating the yarn.lock every now and then if it fixes vulnerabilities and if we get rid of the incompatible module error.

@andyrichardson

This comment has been minimized.

Copy link
Contributor Author

andyrichardson commented Mar 18, 2020

But maybe we should keep generateUserDepsFile async because we can, and since writing the user deps file is done whenever the user saves a relevant source file, the performance gain of async IO might matter. What do you think?

Makes a lot of sense - fixed!

With the lockfiles, I think a seperate PR is a good shout. I wouldn't treat it as anything more than a snapshot of dependencies being used at a specific point in time (useful for getting deterministic results between dev, CI and Prod environments).

Using the lockfile as the source of truth for dependency versions indefinitely I would vouch against because you get the equivalent result of just never updating transient dependencies until a minor bump.

@skidding skidding merged commit 38ca09c into react-cosmos:master Mar 18, 2020
3 checks passed
3 checks passed
ci/circleci: node-v10 Your tests passed on CircleCI!
Details
ci/circleci: node-v8 Your tests passed on CircleCI!
Details
codecov/project 71.45% (+0.1%) compared to c99c364
Details
@skidding

This comment has been minimized.

Copy link
Member

skidding commented Mar 18, 2020

Done 馃帄

Thank you for your contribution!

Please test the new sync APIs in react-cosmos@5.1.0.

And once we know the APIs work as expected we should mention the new server-side APIs in the docs.

@andyrichardson andyrichardson deleted the andyrichardson:add-sync-utils branch Mar 20, 2020
@andyrichardson

This comment has been minimized.

Copy link
Contributor Author

andyrichardson commented Mar 20, 2020

@skidding this looks to be working, thanks for getting it merged so quickly!

@skidding

This comment has been minimized.

Copy link
Member

skidding commented Mar 20, 2020

@andyrichardson Awesome!

I'm curious to learn more about your use case. I think there is potential in offering a more approachable way to create snapshot tests with React Cosmos. If you're interested, we could evolve your setup into a public example or even a standalone package (that you can have ownership over) if that makes sense.

A few thoughts popped into my mind looking at your code:

  • What does the fixtures data structure look like? I expected either getFixtureUrls or getFixtures to be needed at once, but you combine both so I wonder what better APIs could RC provide.
  • Maybe not applicable, but instead of replacing the generated URLs based on env, you could override the hostname and port config options based on env, so that the URLs come out correct from the start.
  • Have you considered making the screenshots artifacts in CircleCI? I would be nice to be able to view the generated screenshots in the build page.
@andyrichardson

This comment has been minimized.

Copy link
Contributor Author

andyrichardson commented Mar 23, 2020

@skidding thanks for looking into this - prior to having sync utils, I wanted to try using backstop but wasn't able to because it didn't support async setup. That would have been my go-to and logic would have been pretty much the same.

In fact, it would have been like this only replacing writing to a file with a module.export in backstop.js.

I expected either getFixtureUrls or getFixtures to be needed at once, but you combine both so I wonder what better APIs could RC provide.

This was because I wanted to get the fullscreen url for each snapshot, and also use the name of the snapshot in the resulting file/test. Two in one might have been useful!

type Fixture = {
  url: string;
  fullscreenUrl: string;
  name: string;
  location: string;
}

Maybe not applicable, but instead of replacing the generated URLs based on env, you could override the hostname and port config options based on env, so that the URLs come out correct from the start.

I will try that!

Have you considered making the screenshots artifacts in CircleCI? I would be nice to be able to view the generated screenshots in the build page.

Right now the workflow is as follows:

  • Take reference snapshots locally
  • Run in CI
  • If it fails in CI, create artifacts from failed diffs

Here's the reference snapshots (including fixture names)

Here's an example failed CI workflow with artifacts

For looking at the visual differences in a PR, we have netlify instances created on PR 馃憤


Hope that helps, hmu on Twitter if you want to jump on a call or talk about this some more 馃憤

@skidding

This comment has been minimized.

Copy link
Member

skidding commented Mar 25, 2020

type Fixture = {
  url: string;
  fullscreenUrl: string;
  name: string;
  location: string;
}

Great idea. The purpose of getFixtures was to be used in "unit" tests where React renders the actual fixture elements. For visual testing purposes the API used should work in a React agnostic environments because you only need fixture meta data to construct URLs to visit and file names to save results under.

So I can definitely see a successor API to getFixtureUrls that gives you all the information you need to construct a visual diff tool over your fixtures. I completely missed the fact that you need clean fixture names to create pretty output :).

Created an RFC for this: react-cosmos/rfcs#14

Here's the reference snapshots (including fixture names)

This is totally out of scope for this discussion, but have you considered alternative storage options for the screenshots? When doing visual diffing in the past we realized that our repo size kept getting bigger over time mostly because of these screenshots, and we ended up storing the snapshots externally (although I don't remember where because it was 3-4y ago). Food for thought.

Here's an example failed CI workflow with artifacts

Beautiful!

For looking at the visual differences in a PR, we have netlify instances created on PR 馃憤

Cool, I'd be curious to see what that UI looks like some day.

Hope that helps, hmu on Twitter if you want to jump on a call or talk about this some more 馃憤

Sure, I'll send you a dm when I have more time to work on this. For now I'll post this reply for posterity.

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.

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