Skip to content
This repository has been archived by the owner on Oct 24, 2021. It is now read-only.

Updates to the build article content for 1.3 #225

Merged
merged 3 commits into from
Mar 1, 2016

Conversation

tmeasday
Copy link
Contributor

@tmeasday tmeasday commented Feb 8, 2016

Still some unanswered questions but I'm interested in your thoughts @stubailo

  

```

XXX: Is it `meteor npm`
XXX: Do we use --save-dev
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be --save for production deps, and --save-dev for development deps, right? Another place where in Meteor the package specifies it (build plugin, debugOnly) and in NPM the user does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean... how does this actually work in practice? I mean the difference between dependencies and devDependencies has a fixed meaning for an npm module which is irrelevant to a meteor app, I would have thought. Unless the build tool respects this somehow?

Copy link
Contributor

Choose a reason for hiding this comment

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

In an ideal world, when you deploy your app it would install your dependencies but not your devDependencies on the production server, so I think there's a valid distinction that a future Meteor tool should respect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no reason why someone couldn't do that right away so I think this makes a lot of sense, thinking about it more.

@stubailo
Copy link
Contributor

stubailo commented Feb 8, 2016

Is there some reason you decided to start documenting the PostCSS package?

@tmeasday
Copy link
Contributor Author

tmeasday commented Feb 8, 2016

Yeah, I thought it was simple enough now and unlikely to change.

@tmeasday
Copy link
Contributor Author

tmeasday commented Feb 9, 2016

Do you think it was a mistake to document it?

@stubailo
Copy link
Contributor

stubailo commented Feb 9, 2016

No I think it's fine! You're right that it's a lot more straightforward when you don't have to worry about meteorhacks:npm.

@tmeasday
Copy link
Contributor Author

I think this is ready to merge @stubailo

@@ -79,7 +79,15 @@ One difference between pre-published packages and local app packages is that the

<h4 id="npm-adding">Adding packages to your app</h4>

Meteor 1.3 will have seamless integration with NPM, and you will be able to simply `npm install` these packages into your app directory. Until then, the easiest way to use NPM packages in your app is [`meteorhacks:npm`](https://atmospherejs.com/meteorhacks/npm).
To install an NPM package into your app, you can simply run NPM's standard install command:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should mention that different versions of NPM do significantly different things, and that some packages might only work with NPM 2, but NPM 3 is better about deduplicating and path length on Windows. In short, we suggest NPM 3 but if that doesn't work for you try NPM 2. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, it's a good point. I don't really feel confident on this exact topic. Do we know NPM3 doesn't work for certain use cases? Because if not, perhaps we can leave it and expect people to just install whatever makes sense for them? It's pretty backwards compatible, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

NPM

It's pretty backwards compatible, right?

Top kek.

No but seriously libraries such as Relay don't work properly with NPM 3 sometimes. This is a fact: facebook/relay#832

Doesn't matter though - I think we should probably recommend NPM 3, and maybe people can figure it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I just leave it or do think we should call out a specific version?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should, since "use NPM" is not specific, since there are three different NPMs that perform in dramatically different ways.

@stubailo
Copy link
Contributor

Just one more comment about NPM 2 vs. 3 and then we are good to go.

@tmeasday
Copy link
Contributor Author

tmeasday commented Mar 1, 2016

Let's merge this; I'll need to reorganise this content once we have a Using Packages article anyway

@stubailo
Copy link
Contributor

stubailo commented Mar 1, 2016

SGTM!

tmeasday pushed a commit that referenced this pull request Mar 1, 2016
Updates to the build article content for 1.3
@tmeasday tmeasday merged commit b381d25 into version-1.3 Mar 1, 2016
@abernix abernix deleted the build-tool-updates-for-1.3 branch June 6, 2017 07:06
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 this pull request may close these issues.

None yet

2 participants