test(bundler): add vite-based tests #3350
Conversation
this commit adds an application that is built using vite to the bundler test directory. it consumes a small stencil library build using the `dist` output target, and verifies that the application can load the web component when the application has been built using vite. this commit does not add the infrastructure to run the tests themselves. that will be completed in a follow on PR
rwaskiewicz
changed the title
this commit adds infrastructure for running the bundler tests in karma. karma was chosen to align with existing parts of our technical stack (see the `test/karma` directory), and to expedite the initial implementation phase of these tests. karma can be difficult to configure, and even more difficult to add new (i.e. different) testing paradigms and testing strategies to. given that these tests do not use browserstack and are a significant departure from the existing karma tests, it felt 'ok' to split these off into a separate set of tests (with their own configuration). in order to get tests up and running, a utilities file, `test/bundler/karma-stencil-utils.ts` has been created. this file is largely based off of `test/karma/test-app/util.ts`. parts of the existing utility file were not ported over if they were deemed unnecessary, and attempts were made to clean up the existing code to improve their readability.
rwaskiewicz
force-pushed
the
rwaskiewicz/bundlers/vite-and-karma
branch
from
dd07967 to
7ece776
Compare
There was a problem hiding this comment.
the new tests pass locally for me with npm run test.bundlers at the root level. the first time I ran it locally I got a timeout, possibly that was some flakines / maybe cache related? anyway - it passed many times after that, so I think it's probably good if CI is passing.
other than that, just had a few small code questions
| */ | ||
| module.exports = function (config: Config): void { | ||
| config.set({ | ||
| browsers: Object.keys(localLaunchers), |
There was a problem hiding this comment.
this is just ["ChromeHeadless"] ? maybe we should define a string constant and use that as the object key as well? like
const CHROME_HEADLESS = "ChromeHeadless"
const localLaunchers = {
[CHROME_HEADLESS]: ...
};sort of 6 of one half a dozen of another though 🤷♀️
| }; | ||
|
|
||
| try { | ||
| const testHtmlRequest = new XMLHttpRequest(); |
There was a problem hiding this comment.
just curious, is there a reason we couldn't use fetch instead of XMLHttpRequest?
There was a problem hiding this comment.
Probably not...TBH I kept the same logic here that is in our existing karma tests. I was hoping (and this is a little wishful thinking) that if the flakiness in our existing Karma tests could be explained by the util file I linked and this subset of it proved to be flaky itself (or not), that might help us debug said flakiness 🤞
| // here, we interpret the empty string to be a valid value | ||
| script.setAttribute('type', typeAttribute); | ||
| } | ||
| } |
There was a problem hiding this comment.
on the attributes here, could we get away with doing something like this?
["src", "type", "nomodule"].forEach(attrName => {
if (tempScripts[i].hasAttribute(attrName)) {
script.setAttribute(attrName, tempScripts[i].getAttribute(attrName))
}
})There was a problem hiding this comment.
I don't think we can, as there are some differences between pulling src directly and calling getAttribute('src') - getAttribute() will return what's exactly in the HTML, whereas the src getter gives us HTML escaping + the full URL to the JS. Take a look at this SO answer + the associated fiddle, the author there gives a good example of the differences between the two.
There was a problem hiding this comment.
ahh that makes sense, I figured there was some subtlety here 👍
test/bundler/karma-stencil-utils.ts
Outdated
| * @param promises the buffer of promises to add instances of `componentOnReady` to | ||
| * @param elm the current element being inspected | ||
| */ | ||
| const waitForDidLoad = (promises: Promise<any>[], elm: Element): void => { |
There was a problem hiding this comment.
should promises have type Promise<d.HTMLStencilElement>[] here? also, do we need to pass it in to the function, since it should be in-scope for waitForDidLoad anyhow?
There was a problem hiding this comment.
should promises have type Promise<d.HTMLStencilElement>[] here?
Yeah - good catch!
also, do we need to pass it in to the function, since it should be in-scope for waitForDidLoad anyhow?
nope, I don't think so - I was originally going to change the scoping here, but thought better of it. Fixed in de7ec55
* fix(runtime): add lazy load for addtl bundlers this commit adds experimental support for using stencil component libraries in projects that use bundlers such as vite. prior to this commit, stencil component libraries that were used in projects that used such bundlers would have issues lazily-loading components at runtime. this is due to restrictions the bundlers themselves place on the filepaths that can be used in dynamic import statements. this commit does not introduce the ability for stencil's compiler to use bundlers other than rollup under the hood. it only permits a compiled component library (that uses the `dist` output target) to be used in an application that uses a bundler built atop of rollup. due to the restrictions that rollup may impose on dynamic imports, this commit adds the ability to add an explicit `import()` statement for each lazily-loadable bundle. in order to keep the runtime small, this feature is hidden behind a new feature flag, `experimentalImportInjection` this pr build's atop the work done by @johnjenkins in #2959 and the test cases provided by @PrinceManfred in #2959 (comment). Without their contributions, this commit would not have been possible. STENCIL-339: Integrate Bundler Functionality * review(ap): remove unnecessary nested `Promise.all` calls remove two nested `Promise.all` calls - after moving writing lazily loadable chunks out of the outermost `Promise.all` call, we were awaiting a single promise wrapped in an array. this commit simplifies the code
Important
This is going to be a part of a chain of PRs. Once all of them are up/approved, I'll merge the child-most PR into its parent, that parent into its own parent, etc. At the end, we shall have a PR (#3349) which contains all of its children, and we'll commit it as one large commit.
As a result, this doesn't have all of the functionality necessary for STENCIL-338 to be complete yet - that'll be incrementally added as we merge the child PRs. This commit adds tests to evaluate the component library (#3349) in Vite, and the infra to run Karma tests
Pull request checklist
Please check if your PR fulfills the following requirements:
npm run build) was run locally and any changes were pushednpm test) were run locally and passednpm run test.karma.prod) were run locally and passednpm run prettier) was run locally and passedPull request type
Please check the type of change your PR introduces:
What is the current behavior?
Stencil does not support component libraries being built with the
distoutput target,then used in an application using a bundler such vite, or parcel
GitHub Issue Number: N/A
What is the new behavior?
Commit 1
this commit adds an application that is built using vite to the bundler
test directory. it consumes a small stencil library build using the
distoutput target, and verifies that the application can load the webcomponent when the application has been built using vite.
this commit does not add the infrastructure to run the tests themselves.
that will be completed in a follow on PR
Commit 2
this commit adds infrastructure for running the bundler tests in karma.
karma was chosen to align with existing parts of our technical stack
(see the
test/karmadirectory), and to expedite the initialimplementation phase of these tests. karma can be difficult to
configure, and even more difficult to add new (i.e. different) testing
paradigms and testing strategies to. given that these tests do not use
browserstack and are a significant departure from the existing karma
tests, it felt 'ok' to split these off into a separate set of tests
(with their own configuration).
in order to get tests up an running, a utilities file,
test/bundler/karma-stencil-utils.tshas been created. this file islargely based off of
test/karma/test-app/util.ts. parts of theexisting utility file were not ported over if they were deemed
unnecessary, and attempts were made to clean up the existing code to
improve their readability.
Does this introduce a breaking change?
Testing
Karma tests for the bundler should now run (locally and as a part of CI).
Other Information
Since this is a part of a chain, please do not use GH to 'Update Branch' - I'll take care of that manually (and be careful not to rebase mid-review cycle).
Upon merging the entire chain, I'll update GH to require the bundler tests to merge (so they won't show up in the list of passed checks in the PR summary down below)