-
Notifications
You must be signed in to change notification settings - Fork 181
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
JS-380 Bundle bridge using esbuild #4901
Conversation
407d8c8
to
02e2a0c
Compare
@@ -0,0 +1 @@ | |||
declare module 'postcss-syntax'; |
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.
fd32e65
to
89c9165
Compare
remove timeout
include: /lib[\/\\]jsts[\/\\]src[\/\\]parsers[\/\\]ast\.js$/, | ||
pattern: [['path.dirname(fileURLToPath(import.meta.url))', '__dirname']], | ||
}), | ||
// Simplify the loader function in babel. At the end it's just importing Babel parser |
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.
Do you think it would be worth to point a specific line in a github repo? Perhaps this way, if something changes, it would be a good starting point to investigate.
Also, it would be great, if we could assert that these replacements actually happened. If we could make the bundling fail if any of the replacements didn't occur....
document sources where bundler is replacing sourcecode
Quality Gate passedIssues Measures |
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.
Commenting with a bit more details, as I'm the author it seems :)
@@ -20,9 +20,6 @@ exports.rules = [ | |||
], | |||
}, | |||
create(context) { |
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.
For posteriority, the lines below were removed because typescript isn't installed and available
@@ -29,13 +29,10 @@ describe('createStylelintConfig', () => { | |||
{ key: 'bar', configurations: [42] }, | |||
]; | |||
const config = createStylelintConfig(rules); | |||
expect(config).toEqual({ | |||
customSyntax: 'postcss-syntax', |
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.
we now pass the imported module and we can't really compare it
@@ -70,7 +70,7 @@ export function buildSourceCode(input: JsTsAnalysisInput, language: JsTsLanguage | |||
return parseForESLint( | |||
input.fileContent, | |||
parser.parse, | |||
buildParserOptions({ parser: vueFile ? parsers.javascript.parser : undefined }, true), | |||
buildParserOptions({ parser: vueFile ? parsers.javascript : undefined }, 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.
Again, we want to pass in a resolved module. parsers.javascript.parser was the library name @babel/eslint-parser
and it would get invoked dynamically and esbuild wouldn't be able to resolve it.
server | ||
.start(Number.parseInt(port), host, createWorker(new URL(import.meta.url), getContext())) | ||
.catch(() => {}); | ||
server.start(Number.parseInt(port), host, createWorker(new URL(import.meta.url), getContext())); |
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.
Consciously we remove the catch. There doesn't seem to be a benefit of silently swallowing the error.
@@ -37,7 +37,7 @@ public class BundleImpl implements Bundle { | |||
|
|||
// this archive is created in the bridge module | |||
private static final String BUNDLE_LOCATION = "/sonarjs-1.0.0.tgz"; | |||
private static final String DEFAULT_STARTUP_SCRIPT = "package/bin/server.mjs"; | |||
private static final String DEFAULT_STARTUP_SCRIPT = "package/bin/server.cjs"; |
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.
@@ -46,7 +46,7 @@ public class NodeCommandBuilderImpl implements NodeCommandBuilder { | |||
|
|||
public static final String NODE_EXECUTABLE_DEFAULT = "node"; | |||
private static final String NODE_EXECUTABLE_DEFAULT_MACOS = | |||
"package/node_modules/run-node/run-node"; | |||
"package/bin/run-node"; |
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.
run-node
is now a part of our project, we don't need to expect it to be in the node_modules
JS-380