Skip to content

update npm package name to edx - #1847

Merged
singingwolfboy merged 1 commit into
masterfrom
db/update-npm-package
Dec 9, 2013
Merged

update npm package name to edx#1847
singingwolfboy merged 1 commit into
masterfrom
db/update-npm-package

Conversation

@singingwolfboy

Copy link
Copy Markdown
Contributor

Part 6 of #1815

@gwprice

gwprice commented Dec 5, 2013

Copy link
Copy Markdown

If this name isn't referenced anywhere else, what is it for?

@gwprice

gwprice commented Dec 5, 2013

Copy link
Copy Markdown

Why is b33bb1a included in this?

@singingwolfboy

Copy link
Copy Markdown
Contributor Author

I think the name is only used if some node package were to depend on edX, which wouldn't make sense. However, it still makes sense to use "edx" instead of "mitx" for the name -- it's less confusing.

As for the extra commit in the PR, I think that's an artifact of the recent, accidental force-push to master (and subsequent fix). I'll rebase it, and that commit should go away.

@gwprice

gwprice commented Dec 5, 2013

Copy link
Copy Markdown

It looks like the only purpose of the package.json file is to install a dev tool (coffee-script). That should be part of dev environment setup, not a rake task.

@singingwolfboy

Copy link
Copy Markdown
Contributor Author

@gwprice Are you suggesting that we remove package.json from the repository, and instead make the rake task just run npm install coffee-script?

@gwprice

gwprice commented Dec 5, 2013

Copy link
Copy Markdown

I don't think the rake task should be doing that; it should be part of the dev environment setup.

@singingwolfboy

Copy link
Copy Markdown
Contributor Author

This seems like the proper way for it to be part of the dev environment setup. Just as we have requirements/edx/base.txt define the Python packages that the environment should contain, package.json defines the Node packages that the environment should contain. I believe that the dev environment scripts already run npm install, which looks for a package.json file and installs all the dependencies that it lists.

So basically, it seems to me like this is the right way of doing it. Do you disagree?

@gwprice

gwprice commented Dec 5, 2013

Copy link
Copy Markdown

I do disagree. The requirements.txt file contains libraries on which we depend and is part of our runtime setup, not dev env setup. coffee-script isn't really a library that we are using, it's a compiler. The way we treat it should be more like ruby or python (i.e. a one-time installation at the time of dev env setup).

@gwprice

gwprice commented Dec 9, 2013

Copy link
Copy Markdown

👍 for now, but this file should go away in the not-too-distant future

@singingwolfboy

Copy link
Copy Markdown
Contributor Author

Fine by me. Merging.

singingwolfboy added a commit that referenced this pull request Dec 9, 2013
@singingwolfboy
singingwolfboy merged commit 724cc02 into master Dec 9, 2013
@singingwolfboy
singingwolfboy deleted the db/update-npm-package branch May 5, 2014 14:46
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