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.

Sign up for GitHub

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

Jump to bottom

[ESLint] Disable on prod instances #8229

Merged
merged 2 commits into from
Feb 15, 2023
Merged

[ESLint] Disable on prod instances #8229

merged 2 commits into from
Feb 15, 2023

Conversation

laemtl
Copy link
Contributor

@laemtl laemtl commented Nov 11, 2022 •
edited
Loading

Prevent ESLint to be run when make is run (prod instances) vs make dev (dev instance).

Note

make steps order were switched to solve a bug with make clean && make (thanks to @maltheism amazing testing skills). The post-install node script runs a tool that requires autoload.php to be generated so composer needs to be run first.

@laemtl laemtl force-pushed the disable-eslint-on-prod branch 3 times, most recently from 566f628 to 77215e6 Compare November 11, 2022 18:47
@laemtl laemtl closed this Nov 14, 2022
@laemtl laemtl reopened this Dec 20, 2022
@laemtl laemtl mentioned this pull request Dec 20, 2022
Closed
maltheism
maltheism reviewed Dec 20, 2022
View reviewed changes
webpack.config.js Outdated
{
from: path.resolve(__dirname, 'node_modules/react/umd'),
to: path.resolve(__dirname, 'htdocs/vendor/js/react'),
flatten: true,
Copy link
Member

Choose a reason for hiding this comment

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

@laemtl flatten: true, shouldn't be here.

maltheism
maltheism reviewed Dec 20, 2022
View reviewed changes
webpack.config.js Outdated
{
from: path.resolve(__dirname, 'node_modules/react-dom/umd'),
to: path.resolve(__dirname, 'htdocs/vendor/js/react'),
flatten: true,
Copy link
Member

Choose a reason for hiding this comment

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

@laemtl flatten: true, shouldn't be here.

maltheism
maltheism requested changes Dec 20, 2022
View reviewed changes
Copy link
Member

@maltheism maltheism left a comment

Choose a reason for hiding this comment

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

See comments.

@maltheism maltheism added the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Dec 20, 2022
@laemtl laemtl removed the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Dec 20, 2022
@maltheism
Copy link
Member

@laemtl conflict

@laemtl laemtl added the Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch label Dec 20, 2022
@laemtl laemtl removed the Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch label Dec 20, 2022
maltheism
maltheism previously approved these changes Dec 20, 2022
View reviewed changes
driusan
driusan requested changes Dec 20, 2022
View reviewed changes
webpack.config.js Outdated
}),
];

mode == 'development' && plugins.push(new ESLintPlugin({
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is the right way of determining this. Your config.xml shouldn't affect your build. Whether you're running make or make dev should determine the build environment.

Copy link
Member

@maltheism maltheism Dec 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@laemtl @driusan one approach would be modify the Makefile to contain:

all: VERSION js
	composer install --no-dev

# If anything changes, re-generate the VERSION file
VERSION: .
	tools/gen-version.sh

phpdev:
	composer install

js:
	npm ci
	mode=production npm run compile

jsdev:
	npm ci
	mode=development npm run compile

jslatest: clean
	rm -rf package-lock.json
	rm -rf modules/electrophysiology_browser/jsx/react-series-data-viewer/package-lock.json
	npm install
	npm run compile

dev: VERSION phpdev jsdev
...

then inside webpack.config.js use const mode = process.env.mode; to obtain whether "development" or "production" and problem solved.

Copy link
Contributor Author

@laemtl laemtl Dec 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@maltheism @driusan Let me know if this new version works for both of you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it looks good to me but what else is affected by the removal of lines 137-149? IIRC they predate this PR..

Copy link
Contributor Author

@laemtl laemtl Feb 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@driusan Those lines set the mode and process.env.NODE_ENV to production/dev, which is implicitly done by passing node-env to webpack (package.json):

"compile": "webpack --node-env=development",
"build": "webpack --node-env=production",

@laemtl laemtl force-pushed the disable-eslint-on-prod branch 2 times, most recently from 8218fb7 to 9e4ee43 Compare December 20, 2022 21:54
@maltheism maltheism self-requested a review December 20, 2022 22:03
@maltheism maltheism dismissed their stale review December 20, 2022 22:03

need to review again

maltheism
maltheism requested changes Dec 20, 2022
View reviewed changes
Copy link
Member

@maltheism maltheism left a comment

Choose a reason for hiding this comment

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

Hi @laemtl the commands make clean && make result in the following:

Warning: require_once(/Users/alizee/Development/GitHub/McGill/Loris/tools/../vendor/autoload.php): Failed to open stream: No such file or directory in /Users/alizee/Development/GitHub/McGill/Loris/tools/get_config.php on line 3
^

SyntaxError: Unexpected token W in JSON at position 1
    at JSON.parse (<anonymous>)
    at Socket.<anonymous> (/Users/alizee/Development/GitHub/McGill/Loris/npm-postinstall.js:79:30)
    at Socket.emit (node:events:513:28)
    at addChunk (node:internal/streams/readable:315:12)
    at readableAddChunk (node:internal/streams/readable:289:9)
    at Socket.Readable.push (node:internal/streams/readable:228:10)
    at Pipe.onStreamRead (node:internal/stream_base_commons:190:23)
npm ERR! code 1
npm ERR! path /Users/alizee/Development/GitHub/McGill/Loris
npm ERR! command failed

Unlike on main I'll get the nice warning: WARNING: Unable to fetch DB config useEEGBrowserVisualizationComponents without it failing to build.

@laemtl
Copy link
Contributor Author

laemtl commented Dec 20, 2022 •
edited
Loading

I believe those issues are fixed in #8293. I rebased the PR, let me know if this works now.

@laemtl laemtl added Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix and removed Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix labels Dec 21, 2022
@ridz1208 ridz1208 added the Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch label Feb 14, 2023
@laemtl laemtl removed the Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch label Feb 14, 2023
@laemtl
Copy link
Contributor Author

laemtl commented Feb 14, 2023

@driusan Rebased

@laemtl laemtl requested a review from driusan February 14, 2023 16:19
@driusan driusan merged commit 47e9c5c into aces:main Feb 15, 2023
@driusan
Copy link
Collaborator

driusan commented Feb 15, 2023

Resolves #5186

@jeffersoncasimir jeffersoncasimir mentioned this pull request Feb 23, 2023
Merged
driusan pushed a commit that referenced this pull request Feb 24, 2023
@jeffersoncasimir
Added node-env specification to npm watch script (#8406)
a572f50 
#8229 introduces a change which causes the npm run watch script to run in production mode.

Added a specifier so that it runs in development mode and I also added the --progress flag, which is very nice to have and useful to me.
@ridz1208 ridz1208 added this to the 25.0.0 milestone Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maltheism maltheism maltheism approved these changes

@driusan driusan Awaiting requested review from driusan

Assignees

@driusan driusan

Labels
None yet
Projects
None yet
Milestone
25.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@laemtl @maltheism @driusan @ridz1208