search

mozilla / tofino

Project Tofino is a browser interaction experiment.
https://mozilla.github.io/tofino
Apache License 2.0
649 stars 67 forks source link

`npm start` runs the build in dev mode #428

Closed victorporof closed 8 years ago

victorporof commented 8 years ago

The way build-config.json is now written has changed. If a property is missing, it's not overwritten as false, but the old value is reused.

victorporof commented 8 years ago

One more reason for https://github.com/mozilla/tofino/issues/429

victorporof commented 8 years ago

@Mossop is there a reason this is the way it is? Why aren't we nuking build-config.json when writing to it in task-config-builder.js? Why are we reusing the old config? I can easily see this getting really messy in the future where we can't easily track which things need to actually be changed to true or false between different kinds of builds.

Mossop commented 8 years ago

We cache the versions of modules that electron-rebuild has made in there and we need to persist that or we'll rebuild more often than we have to. We could cache that somewhere else though.

I thought we were explicitly overwriting with defaults each time already though.

Mossop commented 8 years ago

What effect are you seeing here, Any chance this is just #357?

victorporof commented 8 years ago

I'm seeing npm start running the build in dev mode, instead of production mode.

I can easily fix this by just adding { development: false } in the buildDev task. But I'm very wary of this getting completely out of hand as we keep adding build options. We can't be sure we're overwriting all relevant defaults on every build task forever.

Mossop commented 8 years ago

So the defaults for all values are set in BASE_CONFIG in task-config-builder.js, values there overwrite whatever is in the existing build-config.json and are then overwritten by any properties passed to the config task. Apparently I misread and assumed the default for development was false when I made the changes.

As long as we keep BASE_CONFIG up to date with defaults for any new properties we add then we should be fine. The alternative is to blow things away everytime and do something else to persist the module versions across. I don't mind which really.

victorporof commented 8 years ago

Hmm, that's a good point. But what does 'default' mean? A default for one kind of build should maybe be different from the default of another. Or should we user 'release' as the default build?

Mossop commented 8 years ago

I don't know if it is worth spending too much time on this. I don't think we plan to add much more build settings, we should just set the default to be non-development to solve this and come back to this if we find we're still having issues later.

victorporof commented 8 years ago

sgtm