Skip to content
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

[infra] Remove custom import remapping in tests #2845

Closed
wants to merge 3 commits into from

Conversation

justinfagnani
Copy link
Collaborator

Attempts to fix #2844 for just the lit-html package.

There's a perplexing problem with private-ssr-support.js though, where the identifiers it uses to pull off values from lit-html's private exports object (_$LH) are being minified differently than the identifiers in lit-html.js, leading to private-ssr-support.js export object having incorrect values.

@changeset-bot
Copy link

changeset-bot bot commented May 9, 2022

⚠️ No Changeset found

Latest commit: 534fde5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2022

📊 Tachometer Benchmark Results

Summary

  • lit-element-list
  • lit-html-kitchen-sink
  • lit-html-repeat
  • lit-html-template-heavy
  • reactive-element-list

Results

tachometer-reporter-action v2 for Benchmarks

@justinfagnani justinfagnani changed the title Remove custom import remapping in tests [infra] Remove custom import remapping in tests May 9, 2022
@@ -14,114 +14,142 @@
"type": "module",
"exports": {
".": {
"types": "./development/lit-html.d.ts",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So does this take the place of having to copy types to the root of the package? Can we remove treemirror now? Or do we still need it for back-compat with older TypeScript versions.

}

const browserPresets = {
// Default set of Playwright browsers to test when running locally.
local: [
'chromium', // keep browsers on separate lines
'firefox', // to make it easier to comment out
'webkit', // individual browsers
// 'firefox', // to make it easier to comment out
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uncomment

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm going to put all this back in place. I uploaded and made a draft PR to show what I had done so far and talk about the renaming problem I ran into.

}

const browserPresets = {
// Default set of Playwright browsers to test when running locally.
local: [
'chromium', // keep browsers on separate lines
'firefox', // to make it easier to comment out
'webkit', // individual browsers
// 'firefox', // to make it easier to comment out
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove debugging comment-outs

@@ -176,12 +172,13 @@ export default {
'../lit-html/development/**/*_test.(js|html)',
'../reactive-element/development/**/*_test.(js|html)',
],
nodeResolve: true,
nodeResolve: {
exportConditions: [exportCondition],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@@ -1,57 +0,0 @@
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this file is referenced in a bunch of Wireit dependencies, can all be removed now.

@@ -862,7 +864,7 @@ suite('lit-html', () => {
});

test.skip('renders a Symbol to an attribute', () => {
render(html`<div foo=${Symbol('A')}></div>`, container);
render(html`<div foo=${Symbol('A') as any}></div>`, container);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need the cast now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something with TS 4.7

@@ -19,6 +19,11 @@ import {
} from './lit-html.js';
export type {Template} from './lit-html.js';

const DEV_MODE = true;

console.log('DEV_MODE', DEV_MODE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove debugging

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will when this is ready to go

@@ -19,6 +19,11 @@ import {
} from './lit-html.js';
export type {Template} from './lit-html.js';

const DEV_MODE = true;

console.log('DEV_MODE', DEV_MODE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove debugging

@@ -264,7 +292,7 @@
"output": []
},
"test:prod": {
"command": "MODE=prod node ../tests/run-web-tests.js \"development/**/*_test.(js|html)\" --config ../tests/web-test-runner.config.js",
"command": "MODE=prod node ../tests/run-web-tests.js \"development/**/lit-html_test.js\" --config ../tests/web-test-runner.config.js",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this debugging? We want to keep the *.

@@ -58,7 +58,7 @@
}
],
"settings": {
"typescript.tsdk": "node_modules/typescript/lib",
"typescript.tsdk": "lit-html/node_modules/typescript/lib",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debugging?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just so I don't see errors in VS code that aren't there when building due to the difference TS versions. If we can figure out the renaming issue, this I would upgrade the rest of the repo. This wouldn't be merged as-is.

@augustjk
Copy link
Member

augustjk commented Nov 8, 2023

Superseded by #3132

@augustjk augustjk closed this Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[infra] Remove custom import remapping
3 participants