elidoran / cosmos-browserify

Browserify npm modules for client side in Meteor packages
MIT License
78 stars 12 forks source link

Colon in package name #21

Closed jackgardner closed 9 years ago

jackgardner commented 9 years ago

We have a number of local packages that use cosmos:browserify and after upgrading to Meteor 1.2 we started to receive the following error on build:

While processing files with cosmos:browserify (for target web.browser):
   packages/username:core/client.browserify.js: Cannot find module 'deep-diff'
   from '/Users/dev/Development/app/packages/core/.npm/package'

We managed to trace the problem to here: https://github.com/elidoran/cosmos-browserify/blob/master/plugin/browserify.coffee#L87

We did some googling around the issue and couldn't find anything reported - so if anyone else is also having this problem, the fix was to simply remove the colon from packages/username:core so it became packages/core - but I just wondered what the rationale behind this change was? Is it it expected that the user never includes colons in their local packages?

elidoran commented 9 years ago

when you use meteor add username:packageName to install a package it installs it into app/packages/packageName. It doesn't put the username: into the directory name. I recommend following that convention with local packages for consistency.

Meteor's new Build API changed the way it provides the directory and file info. To get the directory name for a package inside packages I have to strip off the username from the front of the package name provided by the Build API. The Build API should provide the actual directory of the package.

For example, if we have a package username:packageName and a file in the package to browserify at client/package.browserify.js, this is what we get from the old API and the new API:

# # Old API's CompileStep had these properties:

# this isn't the actual directory name, it's the name of the package.
packageName = 'username:packageName'

# this isn't the actual directory either
rootOutputPath = '/packages/username:packageName' 

# doesn't have the directory name here...
inputPath = 'client/package.browserify.js'

# this has the real directory name for the package on the file system
fullInputPath = '/system/absolute/path/to/app/packages/packageName/client/package.browserify.js'

# so, for the old API, I used packageName to know if it was an app or a 
# package I was working on. Then used fullInputPath to get the real directory name.

# # New API's InputPath object has these functions:

# this includes the username, but, that doesn't match what's in the file system
getPackageName() = 'username:packageName'

# this is like the `inputPath` of the old API, it doesn't have the directory
getPathInPackage() = 'client/package.browserify.js'

# this doesn't have the actual directory name either. *and* it has an underscore?
getDisplayPath() = '/packages/username_packageName/client/package.browserify.js'

The new API doesn't provide the actual directory in any of its functions. I have to derive it by stripping off the username from the package name. If you use a local package and put the username and colon on the front of the directory, the values I receive from the Build API are the same as without it. Without a way for me to know, based on the values from the Build API, you're doing something different than the convention, I stick with the conventional format of having no username: in the directory name.

I dug around in the objects provided by the new Build API and I can find the actual directory by looking at an InputFile property: _resourceSlot.packageSourceBatch.unibuild.watchSet.files. The keys in that files object are absolute paths which include the actual directory on the file system. I tested using this and I can get the real directory and allow a directory like username:packageName.

elidoran commented 9 years ago

Although I tested this myself, please let me know if this works for you and I'll publish it in 0.7.1.

jackgardner commented 9 years ago

That fixed the issue - thank you for the prompt response+fix!

elidoran commented 9 years ago

published as 0.7.1.

you're welcome.