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.

Sign up for GitHub

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

Jump to bottom

Platform overrides #94

Closed
wants to merge 12 commits into from
Closed

Platform overrides #94

wants to merge 12 commits into from

Conversation

adam-lynch
Copy link
Contributor

Ok so this resolves #85. See the changes to the README to see how to use this feature.

I'd appreciate it if someone could have a look over this, test and see if I'm missing any use / edge cases or tests.

In particular I'm going to add comments in a couple of places where I think my promise handling doesn't need to be as complex as it is.

So what happens now is:

  • The app manifest has to be read even if a ZIP isn't going to be created.
  • For each target platform, a platform-specific set of options are built if platformOverrides.osx exists in the manifest, for example.
  • For a typical (non-ZIP) Mac build, I just skip the package.json during the copying step and just write a new one instead.
  • If ZIPs need to be created, then as few as possbile are created. If only one of four target platforms has platform-specific overrides, then there will only be two ZIPs created.
  • I never generate a platform-specific manifest and store it temporarily somewhere on disk like I thought I would originally.

cc @steffenmllr, @gabepaez, @dylangattey

adam-lynch
adam-lynch reviewed Oct 24, 2014
View reviewed changes
lib/index.js
}
});

return Promise.all(copiedFiles);
};

NwBuilder.prototype.getZipFile = function(platformName){
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is instead of this._nwFile because we could now have multiple (ZIP) files. Also, this.getZipFile('osx') is a lot clearer than this._nwFile anyway.

@adam-lynch adam-lynch mentioned this pull request Oct 24, 2014
Closed
@steffenmllr
Copy link
Contributor

Nice work, I'll have a look at it over the weekend

@adam-lynch
Copy link
Contributor Author

One downside to this is that it's not supported by node-webkit so if you run nodewebkit to run your app during development, then the overrides won't be applied. So I created gulp-platform-overrides as well (based on platform-overrides which this pull request is based on). If anyone wants to create a Grunt plugin using platform-overrides, then please do.

But thinking about it now, is node-webkit-builder's run option intended for dev yeah? I've never used it. So does that mean that it's suggested the user doesn't use shama/nodewebkit at all? I didn't actually apply the platform overrides when using the run option.

@adam-lynch
Copy link
Contributor Author

Ok, I've decided the run method will not support platform-overrides. It doesn't make sense since run is just meant to take existing files and pass them to node-webkit. It doesn't create a ZIP, temporary files, etc.

The note in the README already covers this;

See #85 for more information. If you need this during development too, see platform-overrides and gulp-platform-overrides.

I guess this means that someone will have to create a Grunt plugin though to wrap platform-overrides for dev.

@steffenmllr did you have a chance to look over it?

@adam-lynch
Copy link
Contributor Author

Once this is merged, I'm also going to see if node-webkit would like this kind of functionality as part of the core.

@steffenmllr
Copy link
Contributor

LGTM.

I also think supporting multiple platforms for run doesn't make sense. You could merge the commits into one feature commit if you like to.

@adam-lynch
Copy link
Contributor Author

Yeah ok, I'll squash & rebase them there now.

@adam-lynch
Copy link
Contributor Author

Ok, after a bit of a battle... they're squashed & rebased.

@adam-lynch
Merge branch 'master' of https://github.com/mllrsohn/node-webkit-builder
afcf6ad 
 into platform-overrides

Conflicts:
	README.md
	package.json
@adam-lynch
Copy link
Contributor Author

I missed a dependency earlier. I commited the fix after I had already pulled in from master, then I tried to squash the fix back with my old commit and now I've mangled everything. It seems some old commits are duplicated :/

@adam-lynch
Copy link
Contributor Author

OK, I ended up having to take a patch from this PR and apply it to a new branch created off master, then squash & rebase again. Merge #99 instead.

@adam-lynch adam-lynch closed this Oct 29, 2014
@adam-lynch
Copy link
Contributor Author

Feature request for support in node-webkit itself: nwjs/nw.js#2542

@adam-lynch adam-lynch deleted the platform-overrides branch November 23, 2014 01:26
@ReadmeCritic ReadmeCritic mentioned this pull request Nov 30, 2015
Merged
@snyk-bot snyk-bot mentioned this pull request May 31, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow tweaks to app manifest per platform
4 participants
@adam-lynch @steffenmllr @y-lohse @yoshuawuyts