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

refactor(server): move filename option from createConfig to API #2113

Closed
wants to merge 3 commits into from

Conversation

knagaitsev
Copy link
Collaborator

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Yes

Motivation / Use-Case

Move default fallback of options.filename out of createConfig, since createConfig is only used on the CLI.

Breaking Changes

If filename is not set, it will now default to the compiler's options.output.filename. However, having filename set will have no effect unless lazy is enabled, so this should not break anything. Does this warrant moving this to next?

Additional Info

@codecov
Copy link

codecov bot commented Jul 10, 2019

Codecov Report

Merging #2113 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2113      +/-   ##
==========================================
+ Coverage   92.56%   92.58%   +0.01%     
==========================================
  Files          33       33              
  Lines        1265     1268       +3     
  Branches      361      363       +2     
==========================================
+ Hits         1171     1174       +3     
  Misses         87       87              
  Partials        7        7
Impacted Files Coverage Δ
lib/utils/createConfig.js 90.99% <ø> (-0.16%) ⬇️
lib/utils/normalizeOptions.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b79f523...a30645e. Read the comment docs.

@alexander-akait
Copy link
Member

/cc @hiroppy what do you think?

@hiroppy
Copy link
Member

hiroppy commented Jul 10, 2019

I think we solve this issue just by moving to lib/utils/updateCompiler.js. Is there any advantage to dividing it into utils other than making it easier to write tests? I think it is a good thing to standardize processing.

@knagaitsev
Copy link
Collaborator Author

I think we solve this issue just by moving to lib/utils/updateCompiler.js. Is there any advantage to dividing it into utils other than making it easier to write tests? I think it is a good thing to standardize processing.

The only reason for the util is to make testing easier.

I was following the style that I did here: #2099. However, I'm not convinced these things really belong in updateCompiler, since they are only using the compiler's data, not changing the compiler in any way.

A more uniform idea that I have is:

I make a helper updateOptions(compiler, options). It will be called in Server.js before anything else, and updateCompiler will be called after it. Its only purpose will be to change properties of the this.options object, like setting defaults and doing essentially what createConfig currently does. Then we will not have to branch out with many different helpers like I was doing in this PR.

@knagaitsev
Copy link
Collaborator Author

/cc @evilebottnawi @hiroppy I switched to using normalizeOptions here.

Do you think I should proceed by working through these options one PR at a time, or by moving many in 1 PR?

@hiroppy
Copy link
Member

hiroppy commented Jul 27, 2019

I'm fine either way. How much is the amount of changes?

@knagaitsev
Copy link
Collaborator Author

I'm fine either way. How much is the amount of changes?

@hiroppy Sorry for late response. I'm essentially just moving everything from createConfig to the new normalizeOptions. I will just put a quick draft together. I might close this PR and combine it with that draft, as this is potential small breaking changes.

@knagaitsev
Copy link
Collaborator Author

closing in favor of #2174

@knagaitsev knagaitsev closed this Aug 6, 2019
@knagaitsev knagaitsev added the gsoc Google Summer of Code label Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gsoc Google Summer of Code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants