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

fix(cli): force a clean build in prestart npm script #6588

Merged
merged 2 commits into from Oct 21, 2020

Conversation

MattiaPrimavera
Copy link
Contributor

@MattiaPrimavera MattiaPrimavera commented Oct 17, 2020

fix build error when manually deleting a model, repository or controller

fix #3259

Checklist

  • DCO (Developer Certificate of Origin) signed in all commits
  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

@@ -52,7 +52,7 @@
<%_ } -%>
"migrate": "node ./dist/migrate",
"openapi-spec": "node ./dist/openapi-spec",
"prestart": "npm run build",
"prestart": "npm run clean && npm run build",
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @mdbetancourt. Do we need to check for yarn version to correctly adapt the template, in this case ?

Yarn 1.x documentation states:

Certain script names are special. If defined, the preinstall script is called by yarn before your package is installed.

whereas as by your link, prestart script needs to be explicitly called for yarn 2 users

@raymondfeng
Copy link
Contributor

@MattiaPrimavera Since you are here, maybe we can further tidy up the scripts as follows:

  1. Add `"rebuild": "npm run clean && npm run build"
  2. Use npm run rebuild for prestart and pretest.
{
  "scripts": {
    "rebuild": "npm run clean && npm run build",
    "prestart": "npm run rebuild",
    "pretest": "npm run rebuild",
  }
}

@raymondfeng
Copy link
Contributor

raymondfeng commented Oct 17, 2020

@MattiaPrimavera Please sign the commit per https://loopback.io/doc/en/contrib/code-contrib.html#signing-off-commits-using-dco.

@MattiaPrimavera
Copy link
Contributor Author

A few questions:

  1. I can't find the yarn version of

    "preopenapi-spec": "npm run build",

script within package.json.ejs, is it expected ?

  1. May it be useful to add a new script, something like:

    "node-source-maps-support": "node -r source-map-support/register ."

to be used within:

<% if (packageManager === 'yarn') { -%>
    "rebuild": "yarn run clean && yarn run build",
    "prestart": "yarn run rebuild",
    "start": "yarn run prestart && node -r source-map-support/register .",
<% } else { -%>
    "rebuild": "npm run clean && npm run build",
    "prestart": "npm run rebuild",
    "start": "node -r source-map-support/register .",
<% } -%>

@mdbetancourt
Copy link
Contributor

mdbetancourt commented Oct 18, 2020

@MattiaPrimavera

"rebuild": "yarn run clean && yarn run build",
"test": "yarn run rebuild && lb-mocha --allow-console-logs \"dist/__tests__\" && npm run lint",
"start": "yarn run rebuild && node -r source-map-support/register ."

it's good enough you dont need to use if-else or pre, post hooks, this is compatible with every package manager,

  1. I can't find the yarn version of
    "preopenapi-spec": "npm run build",
"openapi-spec": "npm run rebuild && node ./dist/openapi-spec",

if you use pre or post hooks it may be executed twice (with npm)

@MattiaPrimavera
Copy link
Contributor Author

To be sure I got your point @mdbetancourt, are you proposing not to use pre & post hooks at all, in order for the generated code to be compatible with all project managers, or only to remove the use of hooks when the packageManager is yarn ?

@MattiaPrimavera
Copy link
Contributor Author

Concerning the following Checklist points:

API Documentation in code was updated

  1. I don't think there's any need for this PR, right ?

Documentation in /docs/site was updated

  1. I guess no need as well

Affected example projects in examples/* were updated

  1. Should I manually update all package.json generated files within examples/* dirs, or do we have an easier and automated way to override the files for all examples ?

@mdbetancourt
Copy link
Contributor

are you proposing not to use pre & post hooks at all

Yes it is

because if you keep hooks for npm and you change the package manager then the user need to change the hooks, also the yarn website provide a good reason to avoid using it

In particular, we intentionally don't support arbitrary pre and post hooks for user-defined scripts (such as prestart). This behavior, inherited from npm, caused scripts to be implicit rather than explicit, obfuscating the execution flow. It also led to surprising executions with yarn serve also running yarn preserve.

@raymondfeng
Copy link
Contributor

Concerning the following Checklist points:

Please only check the ones that apply to your PR.

Affected example projects in examples/* were updated
Should I manually update all package.json generated files within examples/* dirs, or do we have an easier and automated way to override the files for all examples ?

Ideally we should update existing examples to be consistent. There is no automated way but you can use VSCode -> Replace in files to help the editing.

@raymondfeng
Copy link
Contributor

@MattiaPrimavera @mdbetancourt I suggest that we keep this PR simple to only add rebuild and trigger it for start. We can continue to improve yarn experience in another PR. Thanks for chiming in!

@MattiaPrimavera
Copy link
Contributor Author

@MattiaPrimavera @mdbetancourt I suggest that we keep this PR simple to only add rebuild and trigger it for start. We can continue to improve yarn experience in another PR. Thanks for chiming in!

Ok to me @raymondfeng! Thank you @raymondfeng and @mdbetancourt, for your support and fast replies :)

"prestart": "yarn run build",
"rebuild": "yarn run clean && yarn run build",
"prestart": "yarn run rebuild",
"start": "yarn run prestart && node -r source-map-support/register .",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I remove the yarn run prestart call from here then @raymondfeng, in order to address enhancing yarn users experience in a separate PR ?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1. I assume yarn also works with implicit prestart, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes for yarn 1, but it seems that yarn 2 does not instead (pls refer to @mdbetancourt link). I think we currently have an inconsistent behaviour concerning yarn users, since we are using hooks.

Copy link
Contributor

Choose a reason for hiding this comment

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

If that's the case, we can add yarn 1 requirement to engines - see https://classic.yarnpkg.com/en/docs/package-json/#toc-engines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would it be better to address this in a separate issue as well ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is also the reason why @mdbetancourt suggested to avoid the use of pre, post hooks in order to make the behaviour consistent along different project managers

@MattiaPrimavera
Copy link
Contributor Author

What about the occurrences of "pretest": "npm run build" which can be found within:

  • acceptance/
  • extensions/
  • packages/

? Should they be replaced as well with npm run rebuild ?

@raymondfeng
Copy link
Contributor

What about the occurrences of "pretest": "npm run build" which can be found within:
acceptance/
extensions/
packages/

Let's skip them for now.

@@ -24,7 +24,8 @@
"prettier:fix": "npm run prettier:cli -- --write",
"eslint": "lb-eslint --report-unused-disable-directives .",
"eslint:fix": "npm run eslint -- --fix",
"pretest": "npm run clean && npm run build",
"pretest": "npm run rebuild",
"rebuild": "npm run clean && npm run build",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

start and prestart scripts are missing in this file

@MattiaPrimavera
Copy link
Contributor Author

What is the difference between package.json.ejs & package.plain.json.ejs ?

@raymondfeng
Copy link
Contributor

What is the difference between package.json.ejs & package.plain.json.ejs ?

The package.json.ejs is used when loopBackBuild option is selected during lb4 app. Otherwise, package.plain.json.ejs is the template.

@raymondfeng
Copy link
Contributor

@MattiaPrimavera LGTM now. Would you please squash all commits for packages/cli into one and leave updates to existing examples to another one?

…or controller

- force a clean build in prestart npm script
- add rebuild script to more concisely express "clean && build" procedure & remove code duplication

fix loopbackio#3259

Signed-off-by: Mattia Primavera <sconqua@gmail.com>
Add rebuild npm script in example projects & call it within pretest & prestart scripts

fix loopbackio#3259

Signed-off-by: Mattia Primavera <sconqua@gmail.com>
Copy link
Contributor

@raymondfeng raymondfeng left a comment

Choose a reason for hiding this comment

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

Great effort!

@raymondfeng raymondfeng merged commit a4386e9 into loopbackio:master Oct 21, 2020
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment