Skip to content
This repository has been archived by the owner on Dec 13, 2018. It is now read-only.

npm run validate fails on current master #352

Closed
danielberndt opened this issue Oct 26, 2017 · 8 comments · Fixed by #388
Closed

npm run validate fails on current master #352

danielberndt opened this issue Oct 26, 2017 · 8 comments · Fixed by #388

Comments

@danielberndt
Copy link
Collaborator

danielberndt commented Oct 26, 2017

I just checked out the current master and followed the steps from the CONTRIBUTING.md.

Calling npm run validate lead to src/__tests__/theme-provider.js failing:

screen shot 2017-10-26 at 14 02 19

I guess it's due to some dependency not following semver too strictly and now it's representing Components differently.

But then again I'm not too sure.

Given the big amount of dependencies, I'm surprised there's no yarn or npm5 lockfile for glamorous.
As far as I understand, lockfiles within /node_modules/ are ignored (at least with yarn). So having them won't harm users of glamorous, but it'll benefit contributors.

@luke-john
Copy link
Collaborator

Hey @danielberndt, thanks for the detailed issue and looking into this.

This looks similar to an issue in a pr a while ago. If it's the same issue, which I'm pretty sure it is, then it's due to the version of node you are running.

The snapshots unfortunately have different output between v6 an v8.

It would be great if someone could add a note about this to the CONTRIBUTING.md to help other contributors who come across this in the future.

#340 (comment)

@danielberndt
Copy link
Collaborator Author

Ah, alright. Just to give some more details here's my node and npm versions:

  • node: v7.10.0
  • npm: 4.2.0

It's running on macOS Sierra 10.12.6

@luke-john
Copy link
Collaborator

Thanks for providing that information, I just tried locally with node v7.10.0 and can reproduce.

Switching to node v8 and the validate script runs fine.

@kentcdodds
Copy link
Contributor

I should probably fix this so it either works on older versions of node or at least tells you about it...

@mjashanks
Copy link

Hi There, I am also unable to complete npm run validate . Most of my tests fail with the error ...

Enzyme Internal Error: Enzyme expects an adapter to be configured, but found none. To configure an adapter, you should call Enzyme.configure({ adapter: new Adapter() }) before using any of Enzyme's top level APIs, where Adapter is the adapter corresponding to the library currently being tested

I am running

  • Windows 10
  • Node v8.9.4
  • Npm v5.6.0

Any ideas?

@kentcdodds
Copy link
Contributor

I think I have a solution to the original issue of node versions: #388

As for your issue @mjashanks, I was unable to reproduce the problem on a clean clone of the project. Do you mind digging a little further? You'll find that we are configuring enzyme in other/setup-tests.js 🤔 Maybe that's not running for you somehow?

@mjashanks
Copy link

Looked further into this and found a number of issues. Overall this seems to be an issue when building on Windows.

The first issue below is pretty minor - and probably not worth taking further IMO.

  1. ESLint - throws an exception Casing of <modulename> does not match the underlying filesystem . This only happens when you have "cd'd" into the project directory using a different casing than the actual name of the folder...

e.g. My folder name is C:\Code\glamorous and i issued the command cd C:\code\glamorous. Notice the casing of "Code" vs "code". (yes, this is valid in windows :( !)

  1. The rollup kcd-script accesses scripts in cross-env/dist/bin/ directly. These scripts begin with #!/usr/bin/env node, which is unix specific. I have used cross-env on windows, via the package.json (as described in the docs)... so I don't think this is an issue with cross-env. However, I don;t really know all that much about it.

Would like to try and fix this for you, but I don't know enough about rollup and kcd-scripts to start hacking it.

its not a major issue me anymore as @luke-john has already answered solved my original question (in another issue).

If you are going to have a go at fixing this, just let me know an i'll test it for you!

And thanks for getting back so quickly on this an other issues.

@kentcdodds
Copy link
Contributor

Huh, this is interesting. I'm glad that you got things worked out. I'll go ahead and close this issue for now and we can reopen if it becomes an issue in the future or if anyone else wants to dig into it further. Thanks.

kentcdodds pushed a commit that referenced this issue Mar 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants