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

Added a test for #226 #442

Closed
wants to merge 2 commits into from
Closed

Conversation

adambiggs
Copy link
Member

#226 was closed a long time ago, but I think it deserves a second chance.

I think it makes a lot of sense to get the Collection API as close as possible to Model.

@adambiggs
Copy link
Member Author

Hmmm, looks like the source map files are going to make auto-merging pull requests impossible...

@spine Maybe we should add .map files to .gitignore and get people to generate them locally if needed.

@aeischeid
Copy link
Member

yeah, I have been noticing that a large portion of the merge conflicts come from those map files. The .gitignore idea is probably better than what we are doing now, but my thought is it would be nice if they were in the official release stuff that is on npm or that comes from the download link on spinejs.com. Actually, I think I can do that by manually removing the lines from gitignore when prepping for a release.

@adambiggs
Copy link
Member Author

Actually, I think I can do that by manually removing the lines from gitignore when prepping for a release.

Good call. We could make a build script to automate this for release builds as well.

@adambiggs
Copy link
Member Author

Not sure the .gitignore idea is going to work because the .map files are already being tracked...

Makes me wonder how other projects are dealing with source map files.

@cengebretson
Copy link
Member

Maybe take the -m switch off on the CakeFile for build and watch tasks for now (assuming using cake to do the build) and create a separate task like dist as a distribution/release type build that will do final packaging for a release (which includes generating the map files..). Not sure if that is a good final solution, but maybe good enough for now...

@adambiggs
Copy link
Member Author

The down side is source maps will be broken during development if using the build/watch tasks.

@adambiggs
Copy link
Member Author

A final solution for distribution builds might (unfortunately) look like:

  • Un-ignore .map files
  • Run cake build and commit .map files
  • Tag release
  • Re-ignore & delete .map files in another commit right after the release

@cengebretson
Copy link
Member

Another approach...

Can we have a new release branch where the map files are included and then keep master as the active development branch using .gitignore to ignore the map files? When the next version is ready merge master into release and rebuild everything including maps.

This might be more in line with the npm url parameter in package.json too. In that the its suppose to point to the a repo that contains the code that npm is installing.

@adambiggs
Copy link
Member Author

Sounds much better than my idea!

What are your thoughts on creating a dev branch and using master for stable releases?

@aeischeid
Copy link
Member

The dev -> master workflow is one we are very familiar with. I will work on getting this set up

@aeischeid
Copy link
Member

dev branch created, builds there should not produce map files any more and the map files are gone in that branch but exist in master. Master should not be merged into dev. only dev to master. This should isolate the differences in master. Going forward pull requests should be made to the dev branch... I suspect others are going to miss this, but we may be able to dodge that issue if we make dev the default branch for the repo?

@aeischeid
Copy link
Member

by the way I have a pesky little bug with the merge I am trying to do with the chrome_order_fix_merged branch that is holding me up and I was hoping to get done before doing this merge. @cengebretson and @adambiggs I left some debug code in the test and src if you want to take a look.

@adambiggs
Copy link
Member Author

I'll resubmit this PR to dev. Probably is a good idea to make dev the default branch as well.

@adambiggs adambiggs closed this Apr 5, 2013
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.

None yet

3 participants