-
Notifications
You must be signed in to change notification settings - Fork 784
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
Conversation
b3fb8c9
to
dbad1ef
Compare
Note: the pending jobs should properly resolve once merged into |
'@stencil/core/cli': './cli/index.js', | ||
'@sys-api-node': './sys/node/index.js', | ||
|
||
// mapping node.js module names to `sys/node` "cache" | ||
// | ||
// these allow us to bundle and ship a dependency (like `glob`) as part | ||
// of the Stencil distributable but also have our separate bundles | ||
// reference the same file | ||
glob: './sys/node/glob.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.
What's the reason for dropping these? Just not needed in all instances of getEsbuildAliases()
being called?
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.
Correct, some aliases are only needed in certain components e.g. glob
which is why I removed it from here. The alias really depends on where the bundled file is located, e.g. how many levels they are nested within Stencil. Most artifacts are located within a directory, e.g. /testing/index.js
or /compiler/stencil.js
so I kept some global ones but made sure the packages that are located in a more deeper directory structure receive a custom alias.
const sysNodeAliases = getEsbuildAliases(); | ||
const sysNodeAliases = { | ||
...getEsbuildAliases(), | ||
'@stencil/core/compiler': '../../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.
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
to src
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:
- one artifact in
scripts/esbuild/sys-node.ts
- and
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
663118d
to
d2f2e75
Compare
d2f2e75
to
c892100
Compare
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.
Tested building locally and running things in Framework. Can't verify Windows build, however.
What is the current behavior?
Stencil fails when trying to build it on a Windows machine.
STENCIL-1369
What is the new behavior?
It turns out that the alias path needed to get normalized allowing Esbuild to pick up the right file and acknowledge it as external dependency.
This patch will also make some tweaks to the pipeline to run the
build
andlinter
tasks first and then run everything else. Before we were running WebdriverIO immediately.Documentation
Does this introduce a breaking change?
Testing
Updated our pipeline to have the project being build on Windows so we ensure we don't introduce regressions.
Other information
n/a