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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Return Types to Functions #32

Merged
merged 3 commits into from
Oct 12, 2020
Merged

Added Return Types to Functions #32

merged 3 commits into from
Oct 12, 2020

Conversation

shadowtime2000
Copy link
Collaborator

Added return types to functions which gets rid of some ESLint warnings.

@nebrelbug
Copy link
Collaborator

@shadowtime2000 this looks fantastic! Will you quickly run npm run build and npm run format and push the changes? (This will update the Deno build and format using Prettier).

@shadowtime2000
Copy link
Collaborator Author

Getting this error when running npm run build

C:\js-projects\eta>npm run build

> eta@1.11.0 prebuild C:\js-projects\eta
> rimraf dist && rimraf deno_dist


> eta@1.11.0 build C:\js-projects\eta
> denoify && rollup -c rollup.config.ts && typedoc --tsconfig tsconfig.json --target es6 src && deno fmt deno_dist/*.ts

A subdirectory or file -p already exists.
Error occurred while processing: -p.
C:\js-projects\eta\node_modules\denoify\bin\denoify.js:6
process.once("unhandledRejection", error => { throw error; });
                                              ^

Error: Command failed: mkdir -p deno_dist\README.md
A subdirectory or file -p already exists.
Error occurred while processing: -p.

    at checkExecSyncError (child_process.js:630:11)
    at Object.execSync (child_process.js:666:15)
    at execSync (C:\js-projects\eta\node_modules\scripting-tools\dist\lib\index.js:151:26)
    at execSyncNoCmdTrace (C:\js-projects\eta\node_modules\scripting-tools\dist\lib\index.js:177:28)
    at Object.fs_move (C:\js-projects\eta\node_modules\scripting-tools\dist\lib\index.js:528:13)
    at C:\js-projects\eta\node_modules\denoify\lib\denoify.js:154:41
    at Array.forEach (<anonymous>)
    at Object.denoify (C:\js-projects\eta\node_modules\denoify\lib\denoify.js:154:14) {
  status: 1,
  signal: null,
  output: [
    null,
    '',
    'A subdirectory or file -p already exists.\r\n' +
      'Error occurred while processing: -p.\r\n'
  ],
  pid: 5860,
  stdout: '',
  stderr: 'A subdirectory or file -p already exists.\r\n' +
    'Error occurred while processing: -p.\r\n'
}
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! eta@1.11.0 build: `denoify && rollup -c rollup.config.ts && typedoc --tsconfig tsconfig.json --target es6 src && deno fmt deno_dist/*.ts`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the eta@1.11.0 build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\ahuja\AppData\Roaming\npm-cache\_logs\2020-10-10T04_23_15_618Z-debug.log

@nebrelbug
Copy link
Collaborator

@shadowtime2000 hmm, it looks like you're running on Windows -- is that correct?

It seems one of Eta's dev dependencies uses mkdir -p, and the -p flag only works on Unix systems. Do you have WSL installed by any chance? Or maybe a bash shell?

@shadowtime2000
Copy link
Collaborator Author

@nebrelbug Yes, that is correct. I have Git Bash - I don't know if that would be sufficient. It still gives me the same error because I accidentally created a -p directory. I deleted that directory and then I ran the build process again, I got roughly the same error, except it was for a different command that denoify was running.

@shadowtime2000
Copy link
Collaborator Author

Running denoify on its own seems to be working, but it's changing more files than I changed on my own. (mod.ts & browser.ts)

@shadowtime2000
Copy link
Collaborator Author

@shadowtime2000 this looks fantastic! Will you quickly run npm run build and npm run format and push the changes? (This will update the Deno build and format using Prettier).

@nebrelbug Done!

Copy link
Collaborator

@nebrelbug nebrelbug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic! I'm glad you were able to get the build working

@nebrelbug nebrelbug merged commit 99e9f92 into eta-dev:master Oct 12, 2020
@nebrelbug
Copy link
Collaborator

@all-contributors please add @shadowtime2000 for code and ideas

@allcontributors
Copy link
Contributor

@nebrelbug

I've put up a pull request to add @shadowtime2000! 🎉

@nebrelbug
Copy link
Collaborator

nebrelbug commented Oct 12, 2020

@shadowtime2000 I just invited you as a contributor to the repo :)

@shadowtime2000
Copy link
Collaborator Author

@nebrelbug btw just so you know collaborators still have to open a PR because github requires the maintainer approval and travis ci checks.

@nebrelbug
Copy link
Collaborator

@shadowtime2000 I think we want to keep this behavior, since it makes it easy to see and review all changes to master. You should be able to push to any branches other than master 😃.

I think it's also best to wait for a review from the other maintainer, but you should also be able to approve it with your account -- does that work?

@shadowtime2000
Copy link
Collaborator Author

@shadowtime2000 I think we want to keep this behavior, since it makes it easy to see and review all changes to master. You should be able to push to any branches other than master 😃.

I think it's also best to wait for a review from the other maintainer, but you should also be able to approve it with your account -- does that work?

I was just wanting to check what is the purpose of making people maintainers.

@nebrelbug
Copy link
Collaborator

@shadowtime2000 right now you have write access to the repository 😃

This means that you have unrestricted access to all branches except master. You should also be able to review PR's and merge them into master.

The restriction on master just means that you have to create a PR, accept, and merge it rather than committing straight to the branch. This allows other maintainers to look it over and hopefully provides a bit of a buffer to prevent against accidental commits 😂.

This is the strategy that a lot of projects like Docusaurus use. I hope it works well, but feel free to let me know if there are any issues 👍

Thanks for your great contributions & work as a maintainer!

@shadowtime2000
Copy link
Collaborator Author

@nebrelbug I haven't actually looked much at the Docusaurus repo but since it is maintained by a large company, I am going to assume their are a lot of maintainers, whereas here, as I have looked, there are only 2 (possibly wrong). I think the system is more fit for a large project because then the response would be a faster on a large project when there are 100 different people who could respond. Just making that point, though I really don't have much against it.

@nebrelbug
Copy link
Collaborator

@shadowtime2000 that's a valid point.

I just added you to the eta-dev team and gave you Maintainer permissions, so you should be able to push without creating PR's 👍. This should allow you to edit Markdown files, code comments, refactor code, etc. without creating a PR.

For notable code changes, please open a PR (I'll do the same) 😃

Also always make sure to run yarn test before pushing 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants