-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Refactor build scripts and npm package process #5876
Refactor build scripts and npm package process #5876
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@StyleT I wanted to put this up earlier on in case you had any feedback on this approach. Probably just look at the last commit, the rest of them are from the other PRs. |
d71489d
to
2fdad6c
Compare
2fdad6c
to
8c6a75f
Compare
8c6a75f
to
e0b5027
Compare
e0b5027
to
1267509
Compare
1267509
to
a7c7b2e
Compare
a7c7b2e
to
2b992e0
Compare
2b992e0
to
64a414f
Compare
64a414f
to
a814bf1
Compare
|
||
async function updateAllTsconfig() { | ||
const prettierConfig = (await prettier.resolveConfig('./')) || {}; | ||
await updateTsconfig({ |
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 that we shall not generate commited files w/o any mean for people to figure out that they are generated.
More correct approach would be to use the same approach that WXT does within devtools:
- You have tsconfig.json that extends generated config
- You generate "parent" config on postinstall hook and you do not commit it (or you may, but call it
tsconfig.generated.json
)
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.
That's fine, we can add more files if that's the preferred style. I was just following the existing convention.
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.
Extracted to tsconfig*.paths.json 224e201
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.
Awesome! I would still suggest to name it tsconfig*.generated.json
to emphasize that they are not manually written
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.
Had to do a follow-up fix since the jest config expected a specific layout of tsconfig.json 262fed0
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.
it seems that esbuild does not support having an array for the extends argument so we can't have the build json extend both the main tsconfig and a generated file, so I will roll back those changes and someone can revisit splitting up the generated tsconfig separately.
[vite:esbuild] The "path" argument must be of type string. Received an instance of Array
file: /Users/bob/src/lexical/packages/lexical-playground/src/index.tsx
error during build:
TypeError: The "path" argument must be of type string. Received an instance of Array
at validateString (node:internal/validators:162:11)
at Object.isAbsolute (node:path:1160:5)
at resolveExtends (/Users/bob/src/lexical/node_modules/vite/dist/node/chunks/dep-6e2fe41e.js:19612:22)
at parseExtends (/Users/bob/src/lexical/node_modules/vite/dist/node/chunks/dep-6e2fe41e.js:19593:34)
at parse$f (/Users/bob/src/lexical/node_modules/vite/dist/node/chunks/dep-6e2fe41e.js:19544:24)
at async loadTsconfigJsonForFile (/Users/bob/src/lexical/node_modules/vite/dist/node/chunks/dep-6e2fe41e.js:19933:24)
at async transformWithEsbuild (/Users/bob/src/lexical/node_modules/vite/dist/node/chunks/dep-6e2fe41e.js:19736:36)
at async Object.transform (/Users/bob/src/lexical/node_modules/vite/dist/node/chunks/dep-6e2fe41e.js:19821:32)
at async transform (/Users/bob/src/lexical/node_modules/rollup/dist/shared/rollup.js:22085:16)
at async ModuleLoader.addModuleSource (/Users/bob/src/lexical/node_modules/rollup/dist/shared/rollup.js:22311:30)
} | ||
}); | ||
['examples', 'scripts/__tests__/integration/fixtures'] | ||
.flatMap((packagesDir) => glob.sync(`${packagesDir}/*/package.json`)) |
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.
So if we use real packages as fixtures - why can't we simply download them as zip via npm pack
?
And if those are not real NPM packages - better give them more of a semantic naming so it's clear what exactly are we testing here with every one of them.
Alternatively - we can put them as examples and add to docs. So it's not a "dead" code.
Imagine if one of them breaks test - that would be quite a challenge for anyone w/o context to figure out what's going wrong or how to fix it.
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.
Also if aim is to simply test that Lexical exports code correctly - we can simply use Jest with prebuilt Lexical to test for concrete exported API
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.
They are real application packages, but not published to npm. Adding them as zip files would be something that could be done but then it is even harder to see why and how the tests broke. We can add them to examples but I did not do that here because I didn't want to have the discussions about whether these are officially endorsed or not since there are third party packages for some of these frameworks that offer better integration.
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.
They are real application packages, but not published to npm. Adding them as zip files would be something that could be done but then it is even harder to see why and how the tests broke. We can add them to examples but I did not do that here because I didn't want to have the discussions about whether these are officially endorsed or not since there are third party packages for some of these frameworks that offer better integration.
Makes sense. Then can we give them more of a semantic naming so it's clear what exactly we test with every one of them. I'm puzzled why exactly svelte/astro/etc is important here.
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'm puzzled why exactly svelte/astro/etc is important here.
Tons of people use these frameworks in open source
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 would be open to cleaning up the test fixtures for these frameworks and making them "full" examples, but the goal of adding them here was to prevent future compatibility regressions since I've caused and fixed so many along the way to getting esm support. It is really hard to test so many combinations of frameworks that all have their own special way of using webpack/swc/esbuild/vite/etc. unless it's already in the monorepo test suite.
Angular would be a good one to do as a follow-up. The choice of next.js, sveltekit and astro were made because we are not testing them with examples in-tree (we are only testing a basic vite config with and without react), and we have gotten issues or discord problem reports about those frameworks specifically. I had them sitting around for my own testing.
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 we are testing with them right now is simple:
- does lexical build with this framework
- did lexical render (tested by starting the build product & checking with playwright)
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.
Ok, got it. Sounds good to me.
Out of curiosity:
Can you give some example of any change to Lexical that won’t break vanilla js integration but will break any of these “non-React” frameworks
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.
The regressions that I noticed were mostly around changes to the package.json files and the fork modules. Rather than vanilla js vs. non-react frameworks it's usually of a question of vite vs swc vs webpack plus whatever configuration to those bundlers come with the framework.
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.
#5823 is a concrete next.js issue that this test would've caught
test('install & build succeeded', () => { | ||
expect(true).toBe(true); | ||
}); | ||
test(`installed lexical ${monorepoVersion}`, () => { |
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.
@StyleT this is the part of the integration tests that check to make sure that the node_modules of the example project or integration test fixture includes the latest version of lexical (at least up to the version number). There isn't really anything especially unique about the build artifacts to test against since it is trying to simulate what would happen if the commit of lexical under test was released to npm and then third party projects tried to use it
Addresses refactoring of npm/build processes per #5869
README.md
anddocs/packages/{package}.md
files (npm run update-docs)RE #5869 this mostly addresses the "Reduce number of changes needed to add new package" - since I have not added a new package myself yet I think another maintainer (@StyleT?) could add those docs to the guide using this as a baseline?
I was also thinking about maybe starting on an eslint plugin in the next week or two to help with the "rules of $functions" so if someone else doesn't get to it first I could add that documentation then when I am more familiar with all of those details.
Closes #5940 (parts of this PR were extracted there to maybe expedite review)