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

include browserified and browserified+uglified version in dist #476

Merged
merged 1 commit into from Dec 18, 2015
Merged

include browserified and browserified+uglified version in dist #476

merged 1 commit into from Dec 18, 2015

Conversation

dominykas
Copy link
Contributor

Fix for #473, also add source maps, next to browserified JS files. The source maps include sourceRoot pointing to the raw files on Github for the current git HEAD - uglify for some reason then lists sources twice - relative to the project root and then repeats them at sourceRoot - not sure if that's a bug in uglify or not, but I tested in the browser and since sourcesContent are included in the actual source map, things just work.

A couple of questions:

  1. Why is browserification done during prepublish AND during preversion? I wouldn't normally care, but npm version creates a new commit, so it would probably be best to run the build step postversion, as it includes a link to the commit in the source map (event if the content at both commits is roughly the same, for source map purposes). Or maybe prepublish is enough and preversion should be removed?
  2. Should I move es6-shim to dist? That might be considered a breaking change (file no longer where it used to be)
  3. I can also add a source map for the es6-shim, but it seems the output is checked into git? Is it supposed to be that way? Should source map also be checked in?
  4. Installation docs need a rewrite - I can write up the "install with npm, then include the file from dist and use a global" bit, but I don't use bower, so I'm useless there...
  5. npm install reordered the devDependencies - should I put it back the way it was?

@dominykas
Copy link
Contributor Author

Had a couple of broken builds - I'll squash after all the work is complete / questions answered.

@briancavalier
Copy link
Member

so it would probably be best to run the build step postversion, as it includes a link to the commit in the source map

The reason I was doing it preversion is that I actually disable npm's autocommit behavior. Doing it postversion seems perfectly fine to me.

Should I move es6-shim to dist? That might be considered a breaking change

Yeah, I don't think we can do that without bumping to 4.0.0. Even though it's not as tidy as we'd like, let's keep the es6-shim where it is for now to avoid the breaking change.

I can also add a source map for the es6-shim, but it seems the output is checked into git

Yes, it's intentionally committed to github. I'm cool with your adding a source map for it and committing that too.

I can write up the "install with npm, then include the file from dist and use a global" bit

Great, thanks!

but I don't use bower, so I'm useless there...

I'm not much of a bower user anymore, but I can try to help out here. It seems like there might be some tension around what the value of "main" should be in bower.json once a browser dist version and a UMD version are available in the same bower installation. Not sure what to do about that, but I'll try to figure it out.

npm install reordered the devDependencies - should I put it back the way it was?

Nah, don't worry about it.

@dominykas
Copy link
Contributor Author

updated docs and rebased - anything I missed?

@dominykas
Copy link
Contributor Author

@briancavalier ping :)

@briancavalier
Copy link
Member

This looks great, @dominykas. I pulled the branch and tried it out, and both npm run browserify and npm pack afterward seemed to work perfectly. I appreciate the attention to detail in the docs, too.

Is there anything else you want/need to do here, or is this ready to be merged?

@dominykas
Copy link
Contributor Author

Nope, I think this is all set as far as I'm concerned

@briancavalier
Copy link
Member

👍

briancavalier added a commit that referenced this pull request Dec 18, 2015
…-in-dist

include browserified and browserified+uglified version in dist
@briancavalier briancavalier merged commit c3297a2 into cujojs:master Dec 18, 2015
@dominykas dominykas deleted the browserified-output-in-dist branch December 22, 2015 19:19
@dominykas
Copy link
Contributor Author

Hmmm. I can see the new version checked into git, but it's not on npm yet? Anything I can help with?

@briancavalier
Copy link
Member

Sorry for the delay. Here it is

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