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
Code base does not "just work" when cloned (on Windows?) #881
Comments
Thank you for trying to contribute to emotion!
|
You're welcome :) Running on Windows. I'm running the commands from the "Git for Windows" bash prompt. (This one). A brief attempt on Powershell, and in the VSCode Terminal also give similar behaviour (have not compared error outputs to confirm that they're identical, but certain they both give pages and pages of errors. I've run From a brief skim through, a bunch of the snapshot errors are "the emotion hash has changed", but there's also a lot of:
A bunch of these:
And numerous other types of errors. |
Is there anything else useful I can provide? |
It looks like some babel-related thingy is not properly installed... let me check it in Windows (it will take some time since I currently cannot use my Windows machine) |
Oh, and what's the version of your |
node version: v10.10.1 |
Thank you :) |
yarn was installed this morning. node & npm were updated ~ 2 weeks ago? |
@Brondahl I found the problem. Lines 11 to 14 in 0fdafbe
Line 42 in 0fdafbe
Those and some babel-plugin-emotion code also produce different results in Windows (because of path separators), for example,
@mitchellhamilton Could you check this? |
So we're saying that it's not (currently) possible to contribute from a Windows machine? (and that all recent contributors have been running from Linux) I assume your point about path separators is about the direction, not the number? - that we WANT a single backslash rather than a single forwardslash, but that requires escaping the backslash into a double backslash. |
Assuming I've understood correctly, then we should presumably: Assuming it does work then (once @mitchellhamilton has cross-checked,) we should try out something to standardize the slashes across OSs. This SO question has a variety of answers, including citing a node library to handle it: https://stackoverflow.com/questions/125813/how-to-determine-the-os-path-separator-in-javascript I'm happy to raise a PR for that, before my intended one. |
Yes, what I meant is just single |
hmmm ... so I've changed those 2 lines in the babel.config.js So this is definitely an improvement, but there are definitely also other issues :( I'm still getting that huge block of peer dependencies, and the test summary is now:
updated Output file (7,800 lines, 312KB) It looks like all failing tests are now snapshot failures, so that's progress. list of snapshot mismatches, with line numbers indexing into previous file (855 lines, 31KB) Almost all of them are "the hashed classname thing changed". e.g.
but a small handful are path separators:
|
Also, there are a whole bunch of Console.error and console.warn lines in the test outputs, plus warnings about skipped stuff. (e.g:
I'm not clear on whether those are to be expected or not? Could one of the active developers (for whom the code is known to be 'working') run Ta :) |
Here it is 😄 (I used node 8.11.4 since it is the recommended version by https://github.com/emotion-js/emotion/blob/master/.nvmrc) |
I'm not really sure what the next steps are for this? :( I have my actual PR ready-to-go, except for running my tests. I've added the relevant additional tests, and a first guess at what the snapshot tests will look like, so I'm probably ~20 minutes away from raising a pull request once I can run stuff, does anyone have any advice on how to progress on this? |
@Brondahl Should work on Windows now :) |
@Andarist , @Ailrun It looks like it may be the same issue - paths with the wrong slash. I'll post another output file shortly. |
Hmmm... interesting. Here's the output: What's interesting is that it looks like the tests do complete and pass:
and then after the tests have run, then it spits out a whole bunch of Flow errors:
I've never interacted with Flow, though I'm under the impression it's an alternative to Typescript? |
Would u be able to narrow down which testing tasks spits it out? You would have to run them each one by one independently to see which one fails |
So, shortly after this, my laptop froze up, required a power-cycle to use again, and in the process managed to brick the git repo (as in, it didn't know that the folder was a git repo, despite the presence of a I didn't even know the latter was possible! Once I'd recloned the repo, everything seems to be working. |
Just to confirm - every test script is working fine for you now on Windows? |
Running Full output file from |
@Andarist Heya, so I had the same issue with the flow errors again today. I new see 3 modes of behaviour: Behaviour pattern is:
The loop can be reset by closing and reopening VSCode Throughout modes $1 and $2, I get loads of lines like this: It seemed like in mode $1 those 3 retries expired before all the unit tests ran and the whole thing crashed. In mode $2, I eventually get to: In mode $3 none of the above happen, we go straight into the errors:
|
Question: (Note: I don't have the faintest idea what flow is doing and how it works. I'm under the impression that it's something types related? |
Flow is a type checking system - similar to TypeScript. https://flow.org/ You can run flow by using
Each of the testing steps can be run on its own by using |
cool. so yes, as I suspected it's the flow stuff, and it's independant of the rest of the test stuff.
Where do you want to go from here? The error message suggests that it could simply be some slashes causing problems with Flow, though, I'm unsure why it ever worked, in that case? Maybe I got Flow into a state where it had given up and was no longer trying? |
The error of $3 looks weird, since we already removed preact-core package. |
Michael might still have some branch with the preact stuff as the removal of it got merged into master very recently. |
How recently are we talking? I updated at the start of the weekend, to confirm that the commit-hook bug was resolved for me. Will pull and rebase now, to check. |
Nope, still getting that :) What does that mean? |
Getting same error? At that commit, preact-core is already gone... |
And yet, that's the output I get :) |
Might be worth trying, but there is high chance that flow is having some problems on windows. |
The errors in #881 (comment) occurs because we removed next-packages directory. |
And I notice that your preact related errors are the same case. Even though we removed preact packages, ... |
|
A new So I agree, that it's probably an issue with non-git-managed files. That's still pretty disappointing though, right? (especially as it's happened twice, separately, in 2 weeks.) Are those sort of changes going to be common, or was this stuff around preact and next-packages a weird edge case that won't come up again? I'll try out jumping back and forth in git history, to see if I can reproduce the problem reliably, and identify the specific files that need cleaning up. Perhaps we can automate that process? |
Hi there,
I've got 8 years experience of professional software development, but only ~4 months of experience on modern JS stack, so it's very possible that this is entirely mistakes or lack of knowledge about yarn / npm on my part, but I would hope that I was capable of following basic Setup steps for an OS project.
Even if the error is with me, it seems likely that the documentation could be improved to cater to other dummies who still want to Contribute, and I'm happy to submit a PR for that ... once I know what the solution is ... :)
I wanted to raise a pretty trivial code feature PR for
emotion
, so I forked and cloned the project. I expected to be able to just run it and have everything work perfectly before I made my changes, but that isn't the case - if you just follow the direct instructions you get a bunch of warnings followed by an enormous collection of errors and test failures.Details:
Versions:
emotion
version: currentmaster
as of 29/09/2018. (looks like npm pkg version 9.2.10. Commit:0fdabe
.)react
version: (N/A: This is a bug with the library development, not its usage)node
version: v10.10.1npm
version: 6.4.1yarn
version: 1.10.1What you did:
git clone
run Yarn installer from here: https://yarnpkg.com/en/docs/install#windows-stable
run
yarn
from the root of the cloned repo.run
yarn test
from the root of the repo.I believe the above sequence represents the instructions as currently given on the CONTRIBUTE page and then ran
yarn
from the root of my clone.What happened:
yarn
(which I assume to be roughly equivalent tonpm install
?) generated the following set of 17 peer-dependency warnings:yarn test
then generated about a minute and a half of scrolling screen errors, with the final punchline:To be clear, all of this is from a blank, untouched
git clone
of a fork of the emotion code base.Reproduction:
N/A
Problem description:
The repo should JustWork when cloned for development, and/or should Document how to get it working properly.
Suggested solution:
My first guess is that to get this all working I need to install some necessary dependencies.
If that's correct then I suspect the correct solution is to declare those as dev dependencies?
Failing that, then we should be providing notes on what is necessary in the CONTRIBUTING.md file.
The text was updated successfully, but these errors were encountered: