-
Notifications
You must be signed in to change notification settings - Fork 17
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
rewire with merged package.json files #68
base: master
Are you sure you want to change the base?
Conversation
… node instead of babel node to run when in development mode
ENV MU_SPARQL_ENDPOINT 'http://database:8890/sparql' | ||
ENV MU_APPLICATION_GRAPH 'http://mu.semte.ch/application' | ||
ENV NODE_ENV 'production' | ||
ENV MU_SPARQL_ENDPOINT='http://database:8890/sparql' |
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.
fixes warnings during build
@@ -24,7 +25,7 @@ RUN chmod +x /usr/src/app/build-production.sh | |||
|
|||
EXPOSE ${PORT} | |||
|
|||
CMD bash boot.sh | |||
CMD ["bash", "boot.sh"] |
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.
fixes warnings during build
ENV LOG_SPARQL_ALL 'true' | ||
ENV DEBUG_AUTH_HEADERS 'true' | ||
ENV LOG_SPARQL_ALL='true' | ||
ENV DEBUG_AUTH_HEADERS='true' | ||
|
||
WORKDIR /usr/src/app | ||
COPY package.json /usr/src/app/package.json |
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.
no need to add a package lock anymore. The service can choose to include/commit one now as the merged package-lock is generated by the mu install script
## Check if package.json existed and did not change since previous build (/usr/src/app/app/ is copied later in this script, at first run from the template itself it doesn't exist but that's fine for comparison) | ||
cmp -s /app/package.json /usr/src/app/app/package.json | ||
## Check if package.json existed and did not change since previous build (at first run from the template itself it doesn't exist but that's fine for comparison) | ||
cmp -s /app/package.json /usr/src/service-package-old.json |
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.
need to saveguard the original now as we will merge service and template together which results in a change of this file
--inspect="0.0.0.0:9229" \ | ||
./app.js | ||
cd /usr/src/dist/ | ||
if [ "$NO_BABEL_NODE" == "true" ] |
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.
less savvy ides have a problem with debugging in babel node. I don't understand why we are running in babel-node again as we have already babelified before, but this is the current behaviour so i'm leaving it unchanged.
added an env_var to tell the container to just use regular node so less savvy ides are also happy
scripts/config.json
Outdated
"arguments": [] | ||
}, | ||
"environment": { | ||
"image": "semtech/mu-javascript-template:2.0.0", |
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.
this has to be the version of the js image as will be published (feature branch?), if you use e.g. 1.8.0, it will not have the install scripts that are required.
scripts/install/run.sh
Outdated
cp /data/service/package.json /app/package.json | ||
docker-rsync /data/service/ /usr/src/app/app | ||
cd /usr/src/app | ||
NODE_ENV=development ./install-dependencies.sh |
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 development so we also have the dev dependencies
fi | ||
|
||
## mu helpers | ||
cd /usr/src/processing/ |
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.
no longer needed! done during the install step
…all so they can get rid of squiggly red lines too
Thank you for these PRs. Work in this area can indeed help to get newcomers started with the template. It's a big one. Note to others: in order to try this, you must build a local image under the name Running the PR locally
FindingsI did not dig too deep into the implementation and chose to share these comments in order not to postpone indefinitely. State of code hintsI could see the built template be installed in node_modules. We must ensure people run user namespaces when running this as this would generate files as root otherwise. This allows to suggest imports in my editor but it did not yet seem to understand the function definition (as it does do for local sources). For some reason it also kept complaining about missing declaration files which it normally doesn't do. Perhaps the typescript language server update is just more aggressive in forcing to use their tooling. Packages for the types have already been written so it shouldn't be hard to include them though I don't think following their lead is a huge negative for the community... Includes built sources of muThe script seems to install the built sources of the Adding the relevant JSDOC for /**
* Escape date string or date object into an xsd:dateTime for use in a SPARQL string.
*
* @param { Date | string | number } value Date representation
* (understood by `new Date`) to convert.
* @return { string } Date representation for SPARQL query.
*/
function sparqlEscapeDateTime( value ){
return '"' + new Date(value).toISOString() + '"^^xsd:dateTime';
}; This may also help in adding the API documentation inside of the template and the constructiond likely looks similar to what exists in the Python ecosystem. Name of installation scriptWe should rename the name of the script ( Installation script argumentsGlancing through the code earlier I think I saw something like a clean step. If that's in the script to be called then it should be mentioned in the arguments in Future tests
|
I would not call the install script node_modules as it is very javascript-specific and maybe other templates will have the same problem. How about install-dependencies? For code hints, there are type definitions available here, but since the template is pure javascript I wanted to keep it this way. Source inclusion and JSDOC can be done of course. I didn't finish wiring the clean step yet. It removes the package-lock and node_modules of the service for you so that you can do a fresh install, cfr |
This allows some less savvy debuggers to work while running a docker container built on the js template in development mode, which allows you to skip having to connect using a chrome-based browser and instead just work in your IDE
It also reworks the way dependencies are installed by merging the template package.json into the service's package.json if it exists. If there is a difference in versions, the template wins and a warning is logged during the installation of the packages.
A mu script
install
is added that installs the dependencies and provides you with a node_modules and package-lock.json file during development.