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

Use package.json to manage dependecies. #3029

Closed
wants to merge 7 commits into from
Closed

Use package.json to manage dependecies. #3029

wants to merge 7 commits into from

Conversation

ris58h
Copy link
Contributor

@ris58h ris58h commented May 10, 2018

CoffeeScript is a dependency now and there shouldn't be such issues as in #3023

But there are some things to add:

  • It would simplify build if we could get rid of shoulda.js and that git submodule and use any other test runner library/framework.

  • I couldn't run Code Coverage Is Code Coverage broken? #3028 so this part remains unchanged. It would be nice to move jscoverage and temp to devDependencies too.

@smblott-github
Copy link
Collaborator

Thanks, @ris58h.

I get this error...

❯ npm install 

> phantomjs@1.9.20 install /home/blott/local/project/vimium/node_modules/phantomjs
> node install.js

Considering PhantomJS found at /usr/bin/phantomjs
Looks like an `npm install -g`
Linking to global install at /usr/lib/node_modules/phantomjs/lib/phantom/bin/phantomjs
Writing location.js file
npm WARN vimium@ No repository field.
npm WARN vimium@ No license field.

added 107 packages from 75 contributors in 1.433s
npm ERR! Object.entries is not a function

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/blott/.npm/_logs/2018-05-11T08_37_40_608Z-debug.log

(The complete error log is 3000 lines, so I'll not post it here.)

I tried with a fresh clone too. Same thing. npm version is 6.0.1.

Separately, this is going to use the global cake and coffee commands, is it not?

@ris58h
Copy link
Contributor Author

ris58h commented May 11, 2018

@smblott-github It's strange. Everything works fine on my machine and travis seems ok too. Could you provide the log?

@smblott-github
Copy link
Collaborator

Here's the log.

@ris58h
Copy link
Contributor Author

ris58h commented May 11, 2018

@smblott-github I believe that problem is related to node. As I can see in the log you have node v6.12.2. I use v9.6.1. Vimium's travis file contains node_js 8.

@smblott-github
Copy link
Collaborator

I believe that problem is related to node...

That's it, thanks. And...

Separately, this is going to use the global cake and coffee commands, is it not?

It seems that npm does indeed arrange that $PATH is set correctly and uses the local copy of cake and coffee.

Could you...

  • fill in the license and repo in package.json (to prevent unnecessary warnings), and
  • note the required version of node in CONTRIBUTING.md, please?

Note -- something good about this... Legacy users with existing clones are unaffected, I think, right?

@ris58h
Copy link
Contributor Author

ris58h commented May 11, 2018

I do not have coffeescript uninstalled globally so it uses coffeescript/cake from local node_modules.

I do not change any js-code so it won't affect the legacy users, I believe.

I would like to see your opinion about the points I described in the first message: Code Coverage and shoulda.js.

@smblott-github
Copy link
Collaborator

It would simplify build if we could get rid of shoulda.js and that git submodule and use any other test runner library/framework.

You really, really do not want to go there!

More seriously, that's a separate matter; so not for this PR. The test framework is really showing its age, but re-working it would probably be a very large job for little pay back.

I couldn't run Code Coverage #3028 so this part remains unchanged. It would be nice to move jscoverage and temp to devDependencies too.

I have no opinion on this.

@ris58h
Copy link
Contributor Author

ris58h commented May 11, 2018

not for this PR

I know. It's related to dependencies so I've just asked.

I have no opinion on this.

OK. I would investigate that later.

One more question: I see that cake is used in pre-commit git hook, but when i try run that shell-script it fails with mktemp: too few X's in template ‘vimium’ because the name of the template must contain at least 3 X according to man mktemp. Is that hook used at all?

@smblott-github
Copy link
Collaborator

Is that hook used at all?

I don't use it (because sometimes I want to commit code with the tests broken).

@ris58h
Copy link
Contributor Author

ris58h commented May 13, 2018

fill in the license and repo in package.json (to prevent unnecessary warnings)

According to npm docs only two fields are required: name & version. I would like to keep things simple and don't add unnecessary fields because Vimium is not going to be published in the npm registry anyway.

note the required version of node in CONTRIBUTING.md, please?

I've done more research about that problem and it seems like it's just an npm-related issue (fixed).

@ris58h
Copy link
Contributor Author

ris58h commented May 21, 2018

@smblott-github Any feedback?

@smblott-github
Copy link
Collaborator

I'll get to this soon.

@philc
Copy link
Owner

philc commented Jan 10, 2020

Related: #3226. Thanks @ris58h. I may incorporate some of this work as part of converting coffeescript to es6.

@philc philc added the dev-ux label Jan 10, 2020
@philc
Copy link
Owner

philc commented May 22, 2020

  1. I've checked in a package.json file in 1f4f4e6 to fix an issue with Travis builds, but I don't think it's actually needed. Our only dependency now is a utils module for node, and puppetteer for dom testing. I may remove it down the line.

  2. Coffeescript is no longer a dependency, now that everything has been ported to JS (Migrate codebase from coffeescript to ES6 #3459).

@philc philc closed this May 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants