-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
migrate remaining cjs files to mjs #20680
Conversation
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.
Use named exports instead of exporting a wrapper object.
It’s a bad practice, breaks esm lazy nature.
Done expert |
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.
Just some FYI notes.
Great work.
LGTM.
@@ -25,7 +25,7 @@ import BaseGenerator from '../base/index.mjs'; | |||
|
|||
import prompts from './prompts.mjs'; | |||
import statistics from '../statistics.cjs'; | |||
import constants from '../generator-constants.cjs'; | |||
import { CLIENT_MAIN_SRC_DIR, SERVER_MAIN_RES_DIR } from '../generator-constants.mjs'; |
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.
We will need to use application variables instead of constants in the future for custom dirs.
const needleClient = require('./needle-client.cjs'); | ||
const constants = require('../../generator-constants.cjs'); | ||
import needleClient from './needle-client.mjs'; | ||
import { CLIENT_WEBPACK_DIR } from '../../generator-constants.mjs'; |
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.
We will need to use application variables instead of constants in the future for custom dirs.
application.MAIN_DIR = MAIN_DIR; | ||
application.TEST_DIR = TEST_DIR; | ||
application.SERVER_MAIN_RES_DIR = SERVER_MAIN_RES_DIR; | ||
application.ANGULAR = ANGULAR; | ||
application.REACT = REACT; |
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.
Will not be exported to clients in the future.
Should be variables.
application.MAIN_DIR = MAIN_DIR; | |
application.TEST_DIR = TEST_DIR; | |
application.SERVER_MAIN_RES_DIR = SERVER_MAIN_RES_DIR; | |
application.ANGULAR = ANGULAR; | |
application.REACT = REACT; |
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.
Fully agree :-)
Co-authored-by: Daniel Franco <dandrfranco@gmail.com>
@DanielFran commited your change, can please you dismiss the review to merge? |
@Tcharl Can you fix the conflicts? |
Migrate remaining cjs files to mjs
Please make sure the below checklist is followed for Pull Requests.
When you are still working on the PR, consider converting it to Draft (below reviewers) and adding
skip-ci
label, you can still see CI build result at your branch.