refactor: use webpack context when notifying CLI for changed files #455
Conversation
lib/utils.js
Outdated
|
|
||
| function buildEnvData(platform, env) { | ||
| return { | ||
| ...env, |
There was a problem hiding this comment.
This syntax was introduced in NodeJS 8 and later, whereas we currently support and recommend NodeJS 6
| context => `!${context}` | ||
| ); | ||
|
|
||
| return [...originalPatterns, ...ignorePatterns]; |
There was a problem hiding this comment.
The old logic spliced the unneeded watch pattern, whereas the new logic adds an exclusion and retains the originalPatterns.
Whenever we have ["app", "app/App_Resources", "!app"] in watch patterns - does this work as expected?
lib/utils.js
Outdated
| const projectDir = getProjectDir(); | ||
| const config = getWebpackConfig(projectDir, env); | ||
| const context = config.context || projectDir; | ||
| const absolutePathToContext = path.resolve(context); |
There was a problem hiding this comment.
I have some concerns about using just path.resolve here because if memory serves correctly the context is listed as "./app" in the config file and the user may launch cli with --path from another current working directory - will that work?
There was a problem hiding this comment.
Launching with --path and --bundle doesn't work in the master branch too. We're using path.resolve for getting the absolute path of the context (here), because webpack expects absolute paths.
There was a problem hiding this comment.
Discussed in person. Now using paths relative to __dirname in the webpack configs and path.relative with projectDir when running the build.
projectHelpers.js
Outdated
| } catch (e) { | ||
| throw new Error( | ||
| `Couldn't load webpack config from ${configAbsolutePath}. ` + | ||
| `Original error:\n${e}` |
There was a problem hiding this comment.
Not a merge-stopper but I'd rather we use require('os').EOL instead of a hardcoded string here
sis0k0
force-pushed
the
sis0k0/use-compilation-context-compiler
branch
from
1b739f2 to
875fd3e
Compare
|
@Mitko-Kerezov, addressed the review comments with the last commits. |
|
test |
depends on NativeScript/nativescript-cli#3423