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

neon-build refactor to make use of dynamic loading #647

Merged
merged 8 commits into from
Dec 9, 2020

Conversation

kjvalencik
Copy link
Member

@kjvalencik kjvalencik commented Dec 4, 2020

The PR is a little large, but includes several related commits:

  • Re-organize neon-build. This commit introduces no functional changes. It re-organizes napi and neon-sys building so they are entirely separate.
  • Re-write the napi build to make use of dynamic loading. It only needs to output the index.node file
  • Update NAPI tests to use a flattened structure to more accurately reflect the pattern outlined in RFC: Streamlined UX rfcs#36
  • Remove the win delay load hook. This is a little sad because it's very neat, but is also no longer necessary. Let me know if you would like to leave it in for future reference.
  • Update electron tests to use NAPI. This was important for testing that win delay load hook is no longer necessary.
  • Add a neon_build::Builder for customizing the index.node location

Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

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

Looks great! I just suggest we rename the Builder API.

output_file: Option<PathBuf>,
}

impl Builder {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Builder is a confusing name since it doesn't describe what it does -- and it also could be confusing because this crate is neon_build but Builder isn't about running a Neon build.

We talked about Setup::options() being the way to construct this thing. I think it reads nicely.

Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

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

:shipit:

@kjvalencik kjvalencik merged commit c1bdfd1 into main Dec 9, 2020
@kjvalencik kjvalencik deleted the kv/build-refactor branch December 9, 2020 18:11
@kjvalencik kjvalencik mentioned this pull request Dec 9, 2020
YXL76 added a commit to YXL76/cloudmusic-vscode that referenced this pull request Dec 15, 2020
YXL76 added a commit to YXL76/cloudmusic-vscode that referenced this pull request Dec 15, 2020
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