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

modules modules modules #312

Merged
merged 13 commits into from
Sep 20, 2018
Merged

modules modules modules #312

merged 13 commits into from
Sep 20, 2018

Conversation

ashleygwilliams
Copy link
Member

fixes #309 plus rather large refactor which is in the first few commits

strategy here is to define several struct for types of packages folks will want to make (common js only, es6 only, es6 + commonjs to start), we'll serialize the Cargo.toml into the format based on what the target specified is

@ashleygwilliams ashleygwilliams added the work in progress do not merge! label Sep 18, 2018
None => {}
}

NpmPackage::ES6Package(ES6Package {
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: I'd probably name this EsModulePackage since I think modules didn't come until after es6 and es6 is also actually es2015 or some such... or something like that, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah that actually makes a bunch of sense, good catch

@ashleygwilliams ashleygwilliams changed the title [WIP] refactor(manifest): refactor npm package into own mod [WIP] modules modules modules Sep 18, 2018
@ashleygwilliams ashleygwilliams added needs review and removed work in progress do not merge! labels Sep 19, 2018
@ashleygwilliams ashleygwilliams added this to the 0.5.0 milestone Sep 19, 2018
@ashleygwilliams ashleygwilliams changed the title [WIP] modules modules modules modules modules modules Sep 19, 2018
if let Some(s) = scope {
self.package.name = format!("@{}/{}", s, self.package.name);
}
let mut files = vec![wasm_file, js_file.clone()];
Copy link
Member Author

@ashleygwilliams ashleygwilliams Sep 19, 2018

Choose a reason for hiding this comment

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

this clone. i hate it but i also love not having a ton of lifetime parameters everywhere. i think it's fine but if we hate it let's talk about ways to make it as clean as possible.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this single allocation that isn't in a loop (or even within any code that we know to be hot) is a problem. 👍

Copy link
Member

@fitzgen fitzgen 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! The granular commits made this a pleasure to review :)

if let Some(s) = scope {
self.package.name = format!("@{}/{}", s, self.package.name);
}
let mut files = vec![wasm_file, js_file.clone()];
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this single allocation that isn't in a loop (or even within any code that we know to be hot) is a problem. 👍

Copy link
Contributor

@mgattozzi mgattozzi left a comment

Choose a reason for hiding this comment

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

This looks good to me besides the one package thing I mentioned below

}

/// Check the data for missing fields and warn
pub fn check_optional_fields(
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to have this be a function in the impl of the package type so that you can just call:

self.check_optional_fields();

rather than feeding it from three separate variables and avoiding possibly calling the function incorrectly?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh! that's a great idea. can do :)

Copy link
Contributor

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

This seems like a good first step! After this though I imagine there will be a follow-up to publish an "isomorphic" package that has both a CommonJS entry point as well as es modules for bundlers?

Some(format!("{}.d.ts", filename))
};

if let Some(s) = scope {
Copy link
Contributor

Choose a reason for hiding this comment

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

This scope handling logic looks duplicated between this and the above?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is all from master but yeah you are right i think, file an issue?

let dts_file = if disable_dts == true {
None
} else {
Some(format!("{}.d.ts", filename))
Copy link
Contributor

Choose a reason for hiding this comment

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

This dts logic looks duplicated with the above?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is all from master but yeah you are right i think, file an issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

i guess i can try to fix it in this PR, might as well

@ashleygwilliams
Copy link
Member Author

@alexcrichton correct, this is just to lay the groundwork for #313

Copy link
Contributor

@mgattozzi mgattozzi left a comment

Choose a reason for hiding this comment

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

These changes look good to me!

@mgattozzi mgattozzi merged commit 1e9d8de into master Sep 20, 2018
@mgattozzi mgattozzi deleted the mod-mod-mod branch September 20, 2018 15:51
@mgattozzi
Copy link
Contributor

Great job @ashleygwilliams!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use module key for ES6 generated assets
4 participants