-
Notifications
You must be signed in to change notification settings - Fork 35
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
Make yarn-run-dev not move queries.json to build folder #162
base: master
Are you sure you want to change the base?
Conversation
package.json
Outdated
@@ -51,7 51,7 @@ | |||
], | |||
"scripts": { | |||
"lint": "eslint src/*.js", | |||
"dev-prod": "comunica-compile-config config/config-default.json > .tmp-comunica-engine.js && ./bin/queries-to-json.js && webpack serve --config webpack.config.js --mode production", | |||
"dev-prod": "comunica-compile-config config/config-default.json > .tmp-comunica-engine.js && ./bin/queries-to-json.js && webpack serve --config webpack.config.dev.js --mode production", |
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.
Are you sure this is correct. AFAICS, webpack.config.dev.js
does not exist?
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.
Forgot to commit my change, fixed now!
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.
Thanks @simonvbrae, looks good!
Can you confirm build and serving works correctly in these cases? (because the CI currently only checks one of these)
yarn run dev
(yarn run dev-prod
should be the same)yarn run build
node bin/generate.js
If yes, I'll merge this right away!
dd0be8b
to
c52a4bc
Compare
c52a4bc
to
e97a6e8
Compare
I have tested all of the options, it's ready for merge! |
webpack.config.js
Outdated
@@ -41,7 41,7 @@ module.exports = [ | |||
path.join(__dirname, './images/sparql.png'), | |||
path.join(__dirname, './favicon.ico'), | |||
path.join(__dirname, './solid-client-id.jsonld'), | |||
path.join(process.cwd(), './queries.json'), | |||
path.join(__dirname, './queries.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.
I'm not sure this is correct, as this may break overriding queries.json when using bin/generate.js
.
Can you double-check this? (as we don't have unit tests for this)
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.
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.
Should I add a unit test for this? And would it be a good start to check if the custom query correctly ends up in queries.json
in the build
folder?
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.
A test would be great indeed! But let's keep this for a separate PR. I'll merge this one already. Thanks for checking!
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'm sorry for raising this point again, but the lacking unit test is making me a bit paranoid right now 😅
Could you try invoking bin/generate.js
with a custom queries config from a completely different directory to check everything works correctly?
Because the usage of __dirname
here would cause the queries.json
file to be loaded from the repo directory, and not from a different directory.
But I could be missing something.
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.
Better safe than sorry!
Calling ./bin/generate -q /home/custom_queries
worked correctly.
The reason it works is that queries.json
seems to be generated in the directory denoted by __dirname
and copied from there to build/
.
I thought, what if queries.json
gets generated in cwd()
, which could be incompatible with using __dirname
in some cases. I tested this in two ways:
I ran bin/generate -q /home/custom_queries
outside of the repo, this failed because it could not resolve babel-loader
, I assume this is normal behaviour?
I also ran .bin/generate
from a subdirectory of the repo. This worked correctly.
Do these tests cover everything?
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 ran bin/generate -q /home/custom_queries outside of the repo, this failed because it could not resolve babel-loader, I assume this is normal behaviour?
That should also work, but probably doesn't in this case because it can't find the node_modules.
Can you try globally installing this package (your dev version), and running the bin like that? (yarn link
should also be able to do that (at least yarn classic, not sure about the newer ones))
You should then be able to run comunica-web-client-generator
from anywhere.
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 also seemed to work: comunica-web-client-generator -q /home/custom_queries
works in any directory and I used a print to make sure it's using my local version correctly.
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 just re-read your previous comment.
The fact that queries.json
is generated in __dirname
is a bit strange.
This means that even when running the command elsewhere, queries.json
will get generated within the directory of the source code.
I would the source code to remain immutable, and only the cwd to get modified.
Do you think it's feasible to restore the old behaviour of this file getting generated in process.cwd()
? This would also simplify debugging on this 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 have restored it and tested, everything seems to work correctly, including custom queries folder via -q
.
@simonvbrae It looks like the CI doesn't pass. Can you check why? |
The error is This could be caused by this line we discussed above, the failing CI runs may be expecting queries.json to be present in CWD. |
Still seems to be failing after your change :/ |
The issue seems to be caused by an absent build folder, my current commit creates the folder, which fixes the CI. But the absent build folder raises a few questions:
I didn't find anything in the documentation suggesting that webpack creates this directory and, in practise, it doesn't seem to create it because the error of build folder not existing happens after the call to I'm not sure as to whether everything is correct within the CI but I am committing what makes the CI pass for now. |
6ab9e80
to
f0d32c6
Compare
This is a draft PR, I am still making changes.
Calling
yarn run dev
would result in the queries.json file being moved tobuild/
after generation resulting in missing data sources and queries in the development server.This PR changes lets that
move
happen outside of webpack so it doesn't happen when runningyarn run dev
, only when runningyarn run build
.