cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Carlos Santana <csantan...@gmail.com>
Subject Re: [DISCUSS] Copying node_modules during platform install
Date Thu, 29 Oct 2015 03:08:00 GMT
Sorry for being late to the party.
+1 to conclusion

But leaving 2 cents

- Using "npm install" or "npm link" is a development time (i.e. cordova
contributors) only, When a product is shipped we should not rely on using
this process to satisfy dependencies
- The way dependencies are manage in npm/node is not controllable by
package.json, most people they think they can by fixing/pinning the
dependencies versions, but what they don't realize is that the dependencies
children/graph don't usually use fixed versions of their dependencies and
so on thru the whole deep graph
- This is why npm-shrinkwrap exist [1] in the first place
- Bundling all dependencies there are multiple benefits
 1. we shipped the exact version of the dependency that we clear with legal
 2. we avoid less headaches for end user to be responsible to install
dependencies on the fly
 3. we shipped what we tested
 4. every end user get's the exact version of the software, an user running
cordova 5.3.3 that install a week ago, using same bytes of software of a
user that installed cordova 5.3.3 yesterday
Making 3 and 4, the most important to me.

[1] https://docs.npmjs.com/cli/shrinkwrap



On Wed, Oct 28, 2015 at 8:37 PM Steven Gill <stevengill97@gmail.com> wrote:

> +1 nice conclusion.
>
> On Wed, Oct 28, 2015 at 5:30 PM, Jesse <purplecabbage@gmail.com> wrote:
>
> > Yes, I believe the cordova-lib case should be linked, and definitely this
> > is where a relative link makes sense, since it's all in the same repo.
> >
> >
> > @purplecabbage
> > risingj.com
> >
> > On Wed, Oct 28, 2015 at 5:28 PM, Dmitry Blotsky <dblotsky@microsoft.com>
> > wrote:
> >
> > > > trust that each platform will be updated
> > > > with a newer cordova-common when it makes sense
> > >
> > > That sounds reasonable to me. I’m removing the link step for
> > > cordova-common in platforms.
> > >
> > > Last question: the cordova-common used by cordova-lib should still be
> > > linked, yes?
> > >
> > > Kindly,
> > > Dmitry
> > >
> > > > On Oct 28, 2015, at 2:44 PM, Jesse <purplecabbage@gmail.com> wrote:
> > > >
> > > > It is completely likely, and maybe even expected that the specific
> > > version
> > > > of cordova-common that sits in any particular platform template can
> be
> > > > different than what is used by cordova-lib.
> > > > They may even be different amongst platforms in a single project.
> > > >
> > > > As long as the lib->platform api is consistent, this is not an issue.
> > We
> > > > use cordova-common solely to reduce duplicated code in our repos; We
> do
> > > not
> > > > use cordova-common to reduce the duplicated code on app developers'
> > > > machines between multiple projects.
> > > >
> > > > re: >> Follow-up: the reason we link them is so that we test master
> > with
> > > > master at all times.
> > > > I would change this so that a platform is tested as a complete
> entity.
> > > Test
> > > > what is in platform-master and trust that each platform will be
> updated
> > > > with a newer cordova-common when it makes sense.
> > > >
> > > >
> > > >
> > > > @purplecabbage
> > > >
> > >
> >
> https://na01.safelinks.protection.outlook.com/?url=risingj.com&data=01%7c01%7cdblotsky%40microsoft.com%7c25dac595182e43deb77b08d2dfe0ee9c%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=9qhpB1FZ258BqXXE0vEepVP%2fR%2bbxgEViwRhTFEDm6sE%3d
> > > >
> > > > On Wed, Oct 28, 2015 at 1:42 PM, Dmitry Blotsky <
> > dblotsky@microsoft.com>
> > > > wrote:
> > > >
> > > >> This use case does mostly affect Cordova contributors. Usually the
> > > >> platforms will come packaged, you’re right. The discussion of
> > > >> packaged-vs-installed is separate, but just as a nod to it: I don’t
> > see
> > > a
> > > >> reason we can’t just automatically call “npm install” in
> > > >> platforms/ios/cordova. Copying over a package.json with fixed
> versions
> > > is
> > > >> no more complex than copying over node_modules.
> > > >>
> > > >> Kindly,
> > > >> Dmitry
> > > >>
> > > >>> On Oct 28, 2015, at 1:33 PM, Steven Gill <stevengill97@gmail.com>
> > > wrote:
> > > >>>
> > > >>> Currently, those modules ship with the platform. Example,
> cordova-ios
> > > >> will
> > > >>> have all necessary modules bundled in. When you create a cordova
> > > project
> > > >>> and add a platform, it takes those modules and moves them into
your
> > > newly
> > > >>> created cordova project. To have the cordova project run `npm
> > install`
> > > >> and
> > > >>> install those modules would require us include those modules in
a
> > > project
> > > >>> level `package.json` file for every cordova project that adds
that
> > > >>> platform. This would confuse developers for sure. I don't think
we
> > > would
> > > >>> have to modify requires to have this work.
> > > >>>
> > > >>> To run `npm install` on those directories would also be very poor
> > > >> practice.
> > > >>> By those directories, I assume you mean
> > > >>> `MYCORDOVAPROJECT/platforms/ios/cordova` (this is where necessary
> > > >>> node_modules from cordova-ios get copied). It would mean we would
> > have
> > > to
> > > >>> have a `package.json` be copied from `cordova-ios` into
> > > >>> `MyCordovaProject/platforms/ios/cordova`. A cordova project would
> > then
> > > at
> > > >>> least have as many package.json files as platforms have been
> added. I
> > > >> think
> > > >>> this is poor practice.
> > > >>>
> > > >>> Your usecase of npm linking modules in platforms
> > > >> (cordova-ios/node_modules)
> > > >>> that then get copied into `MYCORDOVAPROJECT/platforms/ios/cordova`
> is
> > > >>> unique. I still think the best solution is to add some sort of
> check
> > to
> > > >>> make sure you haven't npm linked or handle the npm linked case
when
> > > >>> copying. Guess this problem will arise more often with
> > cordova-common.
> > > >> That
> > > >>> usecase pretty much only affects cordova contributors.
> > > >>>
> > > >>> -Steve
> > > >>>
> > > >>> On Wed, Oct 28, 2015 at 1:18 PM, Dmitry Blotsky <
> > > dblotsky@microsoft.com>
> > > >>> wrote:
> > > >>>
> > > >>>> Is it possible to do “npm install” in those directories
instead?
> Or
> > to
> > > >>>> adjust the path so that require() works with the original
> > node_modules
> > > >>>> directory?
> > > >>>>
> > > >>>> Kindly,
> > > >>>> Dmitry
> > > >>>>
> > > >>>>> On Oct 27, 2015, at 10:31 PM, Steven Gill <
> stevengill97@gmail.com>
> > > >>>> wrote:
> > > >>>>>
> > > >>>>> I don't think we thought of symlinks in this usecase.
Probably
> > worth
> > > >>>> adding
> > > >>>>> in code that checks for symlinks before the copy. I don't
see us
> > > >> removing
> > > >>>>> this copy as the cordova scripts (build, run, install,
etc)
> require
> > > >> those
> > > >>>>> modules.
> > > >>>>>
> > > >>>>> On Tue, Oct 27, 2015 at 9:24 PM, Dmitry Blotsky <
> > > >> dblotsky@microsoft.com>
> > > >>>>> wrote:
> > > >>>>>
> > > >>>>>> Ping. Anyone have any information on this? Is it safe
to "cp
> -r” a
> > > >>>>>> node_modules directory?
> > > >>>>>>
> > > >>>>>> Kindly,
> > > >>>>>> Dmitry
> > > >>>>>>
> > > >>>>>>> On Oct 26, 2015, at 3:06 PM, Dmitry Blotsky <
> > > dblotsky@microsoft.com>
> > > >>>>>> wrote:
> > > >>>>>>>
> > > >>>>>>> Hey folks,
> > > >>>>>>>
> > > >>>>>>> I’ve come across a bug with symlinks and platform
installation
> > > >>>> recently.
> > > >>>>>> The point of interest is this line:
> > > >>>>>>
> > > >>>>
> > > >>
> > >
> >
> https://na01.safelinks.protection.outlook.com/?url=https%3a%2f%2fgithub.com%2fapache%2fcordova-ios%2fblob%2f4039aeb6f87c6803df5814b8cdefb8c2058504a0%2fbin%2flib%2fcreate.js%23L93.&data=01%7c01%7cdblotsky%40microsoft.com%7c2b31253a23cc4d165ed808d2de51c9ef%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=cyXGPBQPr9v46gc6DMDeC9zZkzda8bSCIoinEyAmKrs%3d
> > > >>>>>>>
> > > >>>>>>> Is copying node_modules a safe operation? In my
case there was
> a
> > > >>>>>> relative symlink inside it when it was copied and
as a result
> some
> > > >>>>>> dependencies broke. The symlink was created by a previous
> > invocation
> > > >> of
> > > >>>>>> “npm link”.
> > > >>>>>>>
> > > >>>>>>> Kindly,
> > > >>>>>>> Dmitry
> > > >>>>>>
> > > >>>>>>
> > > >>>>
> > > >>>>
> > > >>
> > > >>
> > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message