fix(cli): force a clean build in prestart npm script by MattiaPrimavera · Pull Request #6588 · loopbackio/loopback-next · GitHub

MattiaPrimavera · 2020-10-17T17:19:01Z

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

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

mdbetancourt · 2020-10-17T18:00:21Z

    "migrate": "node ./dist/migrate",
    "openapi-spec": "node ./dist/openapi-spec",
-    "prestart": "npm run build",
+    "prestart": "npm run clean && npm run build",


pls can you read the following link https://yarnpkg.com/getting-started/migration#explicitly-call-the-pre-and-post-scripts for yarn 2 user

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 · 2020-10-17T19:40:37Z

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

Add `"rebuild": "npm run clean && npm run build"
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 · 2020-10-17T19:41:03Z

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

MattiaPrimavera · 2020-10-18T00:05:36Z

A few questions:

I can't find the yarn version of
```
"preopenapi-spec": "npm run build",
```

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

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 · 2020-10-18T07:35:26Z

@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,

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 · 2020-10-18T13:43:51Z

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 · 2020-10-18T13:56:35Z

Concerning the following Checklist points:

API Documentation in code was updated

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

Documentation in /docs/site was updated

I guess no need as well

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 ?

mdbetancourt · 2020-10-18T19:17:50Z

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 · 2020-10-19T15:32:33Z

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 · 2020-10-19T15:34:39Z

@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 · 2020-10-19T15:59:02Z

@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 :)

MattiaPrimavera · 2020-10-19T16:04:49Z

-    "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 .",


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

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

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.

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

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

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 · 2020-10-19T16:42:15Z

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 · 2020-10-19T16:43:56Z

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

Let's skip them for now.

MattiaPrimavera · 2020-10-20T09:16:42Z

    "eslint:fix": "npm run eslint -- --fix",
-    "pretest": "npm run clean && npm run build",
+    "pretest": "npm run rebuild",
+    "rebuild": "npm run clean && npm run build",


start and prestart scripts are missing in this file

MattiaPrimavera · 2020-10-20T09:23:24Z

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

raymondfeng · 2020-10-20T16:52:04Z

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 · 2020-10-20T16:52:51Z

@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>

raymondfeng

Great effort!

MattiaPrimavera requested review from agnes512 and emonddr as code owners October 17, 2020 17:19

MattiaPrimavera marked this pull request as draft October 17, 2020 17:20

mdbetancourt reviewed Oct 17, 2020

View reviewed changes

dhmlau added community-contribution CLI labels Oct 18, 2020

MattiaPrimavera force-pushed the master branch from e37cee9 to c845e7e Compare October 18, 2020 13:37

MattiaPrimavera commented Oct 19, 2020

View reviewed changes

MattiaPrimavera force-pushed the master branch from c845e7e to 3e60be1 Compare October 19, 2020 16:39

MattiaPrimavera requested a review from raymondfeng October 19, 2020 16:45

MattiaPrimavera commented Oct 20, 2020

View reviewed changes

MattiaPrimavera marked this pull request as ready for review October 20, 2020 09:30

MattiaPrimavera requested review from bajtos, dhmlau, hacksparrow, jannyHou and marioestradarosa as code owners October 20, 2020 09:30

MattiaPrimavera requested a review from nabdelgadir as a code owner October 20, 2020 09:30

MattiaPrimavera added 2 commits October 21, 2020 14:29

fix(cli): update affected example projects

69bd005

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

MattiaPrimavera force-pushed the master branch from b6c54d3 to 69bd005 Compare October 21, 2020 12:38

raymondfeng approved these changes Oct 21, 2020

View reviewed changes

raymondfeng merged commit a4386e9 into loopbackio:master Oct 21, 2020

Conversation

MattiaPrimavera commented Oct 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymondfeng commented Oct 17, 2020

Uh oh!

raymondfeng commented Oct 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MattiaPrimavera commented Oct 18, 2020

Uh oh!

mdbetancourt commented Oct 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MattiaPrimavera commented Oct 18, 2020

Uh oh!

MattiaPrimavera commented Oct 18, 2020

Uh oh!

mdbetancourt commented Oct 18, 2020

Uh oh!

raymondfeng commented Oct 19, 2020

Uh oh!

raymondfeng commented Oct 19, 2020

Uh oh!

MattiaPrimavera commented Oct 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MattiaPrimavera commented Oct 19, 2020

Uh oh!

raymondfeng commented Oct 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MattiaPrimavera commented Oct 20, 2020

Uh oh!

raymondfeng commented Oct 20, 2020

Uh oh!

raymondfeng commented Oct 20, 2020

Uh oh!

raymondfeng left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MattiaPrimavera commented Oct 17, 2020 •

edited

Loading

raymondfeng commented Oct 17, 2020 •

edited

Loading

mdbetancourt commented Oct 18, 2020 •

edited

Loading