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

Allow setting compiler options in node/index.js. #95

Merged
merged 1 commit into from
Apr 6, 2017
Merged

Allow setting compiler options in node/index.js. #95

merged 1 commit into from
Apr 6, 2017

Conversation

jdalton
Copy link
Contributor

@jdalton jdalton commented Mar 27, 2017 •
edited
Loading

This PR is a take at allowing the node/index.js to accept compiler option overrides.

It allows:

require("reify/node")({
  someSetting: true,
  otherSetting: "path/to/smth"
})

The interface could be different though, this is just an initial take on it.

Related to #91.

benjamn
benjamn reviewed Mar 27, 2017
View reviewed changes
node/caching-compiler.js Outdated
@@ -48,7 +48,7 @@ const fallbackPkgInfo = {
cache: Object.create(null)
};

function readWithCache(info, content) {
function readWithCache(info, content, options) {
const json = info && info.json;
const reify = json && json.reify;
const cacheFilename = getCacheFilename(content, reify);
Copy link
Owner

Choose a reason for hiding this comment

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

The new options should probably be included in the hash computed by getCacheFilename, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don' think getCacheFilename is using any options at the moment.

Copy link
Owner

Choose a reason for hiding this comment

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

But the options might affect the compiler output, and we want getCacheFilename to return different names for different versions of the compiled code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oooh, I see what you're saying. Do you want to include that in a patch to simplify the hash bit or do it here?

@jdalton jdalton mentioned this pull request Mar 28, 2017
Closed
@jdalton
Copy link
Contributor Author

jdalton commented Apr 3, 2017

Rebased.

@jdalton jdalton mentioned this pull request Apr 3, 2017
Open
7 tasks
benjamn
benjamn approved these changes Apr 4, 2017
View reviewed changes
node/compile-hook.js Outdated
@@ -5,14 +5,20 @@ const compiler = require("./caching-compiler.js");
const Module = require("./runtime.js").Module;
const Mp = Module.prototype;

let overrideCompileOptions = {};

module.exports = (options) => {
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe also update the exports variable in this assignment, just in case we ever accidentally use that in this module.

Copy link
Contributor Author

@jdalton jdalton Apr 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure what you're asking. Can you give an example. Added it!

@jdalton jdalton merged commit 47eff30 into benjamn:master Apr 6, 2017
@jdalton jdalton deleted the options branch April 6, 2017 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjamn benjamn benjamn approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jdalton @benjamn