-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
feat: move nuxt-legacy and nuxt-start into packages #3824
Conversation
build/package.js
Outdated
const { readFileSync, existsSync, readJSONSync, writeFileSync, copySync } = require('fs-extra') | ||
const builtin = require('./builtin.json') | ||
|
||
module.exports = class Package { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love how clean this is -- could we get this working with ESM syntax though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. We can use node -r esm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
files = source.packageObj.files || [] | ||
} | ||
|
||
for (const file of files) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Widening parenthesis as blocks eliminates the need for extra variables -- since this style is already heavily employed throughout Nuxt, can we also do it for things like this?
for (const file of files) {
copySync(
resolve(source.options.rootDir, file),
resolve(this.options.rootDir, file)
)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've kept variables for reasons:
- No need to add extra comments. The code is much readable when using well-named variables
- Adding complexities (Like more logics for resolving path with some
if
conditions) would require fewer changes to the code - Primarily I've separated variables soi can add some console logs about current operation without repeating code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pi0 yup, I do that too. And fair enough. Usually though when I realize there are no further references to something, I edit it into the call in the final revision. Perhaps we can selectively do that when it makes sense just before merging.
packages/nuxt-start/sync.js
Outdated
|
||
// Construct start package | ||
const start = new Package({ rootDir: __dirname }) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Also @pi0 I've edited out |
Codecov Report
@@ Coverage Diff @@
## dev #3824 +/- ##
=======================================
Coverage 97.84% 97.84%
=======================================
Files 18 18
Lines 1162 1162
Branches 317 317
=======================================
Hits 1137 1137
Misses 24 24
Partials 1 1 Continue to review full report at Codecov.
|
@pi0 have you tested if |
@galvez How is this related to this PR changes? BTW as i remember it should be working fine |
@pi0 I'm not sure, just looked somewhat related. I'll take your word :) |
@pi0 argh, seems esm borked |
@galvez No worries. It was an appveyor problem. Rebuilt fixed problem :) |
build/builtins.js
Outdated
.filter(x => !/^_|^(internal|v8|node-inspect)\/|\//.test(x) && !blacklist.includes(x)) | ||
.sort() | ||
|
||
export const builtinsMap = () => builtins.reduce((obj, builtin) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we leverage Map
here? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I wrote it with Map at the beginning, but object is handy and also map structure underlying,so I changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This is a step towards Lerna style structure of Nuxt.js.
Branch
https://github.com/nuxt/nuxt.js/tree/refactor/packages/packages
Changes
nuxt-start
into a packagenuxt-legacy
into a new packagePackage
utility addeddist.js
script. Package.json simplifiedTODO
add Lerna