Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
fix(scripts): fix Esbuild scripts to allow to run on Windows #5930
fix(scripts): fix Esbuild scripts to allow to run on Windows #5930
Changes from all commits
eaa4ba3
c892100
d28317e
c05c9f8
b8aba28
c8e9df2
2c8d4ae
236446d
471ccfd
7e1545a
d0c34d0
8c442c9
2f16693
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
If this path is different than what's spit out by
getEsbuildAliases()
, does it make sense to have that function generate the path based on some directory? Why are they different in the first place?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 are some build artifacts that located multiple folders deep that require a different alias path, e.g. for
/testing/index.js
the alias for@stencil/core/compiler
has to be../compiler/stencil.js
while for/sys/node/index.js
it has to be../../compiler/stencil.js
.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.
Right, so I was thinking we could figure out how deeply nested we are based on the relative path from
__filename
tosrc
or something and use that info to generate the relative paths.Or, alternatively, remove the helper altogether. What you have works, but feels a bit odd that we use it and then override the returned aliases in quite a few spots.
Open to thoughts!
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.
Most Stencil components can be successfully compiled using the common defined aliases as implemented in scripts/esbuild/utils/index.ts, there are only two artifacts that require modifications to these aliases which is:
scripts/esbuild/sys-node.ts
scripts/esbuild/internal-platform-testing.ts
If you want I can get rid of the
getEsbuildAliases
function and have this defined per component but I think this will create more code than we have right now.Wdyt?
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 think it's fine for now if it's only a couple spots. Thanks for the pushback, no need to overcomplicate