-
Notifications
You must be signed in to change notification settings - Fork 26.8k
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
Add decoratorMetadata flag if enabled by tsconfig #32914
Add decoratorMetadata flag if enabled by tsconfig #32914
Conversation
Maybe also add a |
I agree, enabling
|
This comment has been minimized.
This comment has been minimized.
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.
This is missing an integration test to verify that the change works.
@sannajammeh I've prepared integration tests for you but for some reason I can't push my changes to your PR here. Will you be so kind and add them to this PR? test/development/basic/emit-decorator-metadata.test.tsimport { join } from 'path'
import webdriver from 'next-webdriver'
import { createNext, FileRef } from 'e2e-utils'
import { NextInstance } from 'test/lib/next-modes/base'
import { BrowserInterface } from 'test/lib/browsers/base'
describe('emitDecoratorMetadata SWC option', () => {
let next: NextInstance
beforeAll(async () => {
next = await createNext({
files: {
'jsconfig.json': new FileRef(
join(__dirname, 'emit-decorator-metadata/jsconfig.json')
),
pages: new FileRef(join(__dirname, 'emit-decorator-metadata/pages')),
},
})
})
afterAll(() => next.destroy())
it('should compile with emitDecoratorMetadata enabled', async () => {
let browser: BrowserInterface;
try {
browser = await webdriver(next.appPort, '/')
const message = await browser.elementByCss('#message').text()
expect(message).toBe('Hello, world!')
} finally {
if (browser) {
await browser.close()
}
}
})
}) test/development/basic/emit-decorator-metadata/jsconfig.json{
"compilerOptions": {
"experimentalDecorators": true,
"emitDecoratorMetadata": true,
"jsx":"preserve"
},
} test/development/basic/emit-decorator-metadata/pages/index.tsximport React from 'react';
import 'reflect-metadata';
import { container, singleton } from 'tsyringe';
@singleton()
export class HelloService {
getHello() {
return 'Hello, world!';
}
}
const helloService = container.resolve(HelloService);
export default function Home() {
const message = helloService.getHello();
return <p id="message">{message}</p>;
} Also you need to add tsyringe and reflect-metadata as dev dependencies with the following command: yarn add reflect-metadata tsyringe -D -W |
This comment has been minimized.
This comment has been minimized.
this would be quite awesome when added! thanks so much for your work! |
@abriginets Appreciate these you adding these tests! I have added them to this PR. |
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.
Looks fine to land now if it passes the tests, thanks!
This comment has been minimized.
This comment has been minimized.
This seems to be the only issue currently. Not sure what is causing it as the legacy-decorators test uses the same method without presenting a parser issue. |
I'm assuming it's Eslint |
I guess this is due to eslint using babel as a parser under the hood, isn't it? shouldn't we then be able to add sth like along the lines of this to the eslint config?
|
If it's eslint that is not satisfied with the code you should just add a comment to eslint-ignore it. I'm not sure what Next.js team's opinion is about ignoring linter issues but we do it all the time as long as those issues occurs in test files and not in the actual runtime codebase. |
The particular file can be eslint ignored as it's a test file. Main question this raises for me is if the eslint integration in Next.js will error on this as well 🤔 |
Not sure. I have been running decorators with Next JS 12 without SWC and faced no linter issues. |
I can confirm this. Using decorators with a custom babel config rn without an eslint issue (or any special config there). |
Had to turn off SWC compilation in next.js. Should be fixed in vercel/next.js#32914 Added some babel plugins needed for decoratorMetadata emit Workaround for DB connection failure when dealing with HMR in dev mode Workaround for loading Entity classes (has to have ormconfig.js config overridden in app to get around dynamic import which was causing all sorts of issues with JS module loading)
Is any movement possible here? |
# Conflicts: # package.json
Stats from current PRDefault Build (Decrease detected ✓)General Overall increase
|
vercel/next.js canary | sannajammeh/next.js add/emit-decorator-metadata | Change | |
---|---|---|---|
buildDuration | 15.3s | 15.5s | |
buildDurationCached | 6s | 6s | -20ms |
nodeModulesSize | 359 MB | 359 MB |
Page Load Tests Overall decrease ⚠️
vercel/next.js canary | sannajammeh/next.js add/emit-decorator-metadata | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 3.179 | 3.222 | |
/ avg req/sec | 786.3 | 775.82 | |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 1.328 | 1.353 | |
/error-in-render avg req/sec | 1881.83 | 1847.93 |
Client Bundles (main, webpack, commons)
vercel/next.js canary | sannajammeh/next.js add/emit-decorator-metadata | Change | |
---|---|---|---|
450.HASH.js gzip | 179 B | 179 B | ✓ |
framework-HASH.js gzip | 42.2 kB | 42.2 kB | ✓ |
main-HASH.js gzip | 27.4 kB | 27.4 kB | ✓ |
webpack-HASH.js gzip | 1.44 kB | 1.44 kB | ✓ |
Overall change | 71.2 kB | 71.2 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | sannajammeh/next.js add/emit-decorator-metadata | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | sannajammeh/next.js add/emit-decorator-metadata | Change | |
---|---|---|---|
_app-HASH.js gzip | 1.36 kB | 1.36 kB | ✓ |
_error-HASH.js gzip | 194 B | 194 B | ✓ |
amp-HASH.js gzip | 312 B | 312 B | ✓ |
css-HASH.js gzip | 326 B | 326 B | ✓ |
dynamic-HASH.js gzip | 2.37 kB | 2.37 kB | ✓ |
head-HASH.js gzip | 350 B | 350 B | ✓ |
hooks-HASH.js gzip | 919 B | 919 B | ✓ |
image-HASH.js gzip | 4.94 kB | 4.94 kB | ✓ |
index-HASH.js gzip | 263 B | 263 B | ✓ |
link-HASH.js gzip | 2.19 kB | 2.19 kB | ✓ |
routerDirect..HASH.js gzip | 321 B | 321 B | ✓ |
script-HASH.js gzip | 383 B | 383 B | ✓ |
withRouter-HASH.js gzip | 318 B | 318 B | ✓ |
85e02e95b279..7e3.css gzip | 107 B | 107 B | ✓ |
Overall change | 14.3 kB | 14.3 kB | ✓ |
Client Build Manifests
vercel/next.js canary | sannajammeh/next.js add/emit-decorator-metadata | Change | |
---|---|---|---|
_buildManifest.js gzip | 459 B | 459 B | ✓ |
Overall change | 459 B | 459 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | sannajammeh/next.js add/emit-decorator-metadata | Change | |
---|---|---|---|
index.html gzip | 532 B | 532 B | ✓ |
link.html gzip | 546 B | 546 B | ✓ |
withRouter.html gzip | 526 B | 526 B | ✓ |
Overall change | 1.6 kB | 1.6 kB | ✓ |
Default Build with SWC (Increase detected ⚠️ )
General Overall increase ⚠️
vercel/next.js canary | sannajammeh/next.js add/emit-decorator-metadata | Change | |
---|---|---|---|
buildDuration | 18.7s | 19s | |
buildDurationCached | 6s | 6s | -14ms |
nodeModulesSize | 359 MB | 359 MB |
Page Load Tests Overall increase ✓
vercel/next.js canary | sannajammeh/next.js add/emit-decorator-metadata | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 3.174 | 3.172 | 0 |
/ avg req/sec | 787.6 | 788.19 | +0.59 |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 1.324 | 1.315 | -0.01 |
/error-in-render avg req/sec | 1888.56 | 1900.5 | +11.94 |
Client Bundles (main, webpack, commons)
vercel/next.js canary | sannajammeh/next.js add/emit-decorator-metadata | Change | |
---|---|---|---|
450.HASH.js gzip | 179 B | 179 B | ✓ |
framework-HASH.js gzip | 42.3 kB | 42.3 kB | ✓ |
main-HASH.js gzip | 27.4 kB | 27.4 kB | ✓ |
webpack-HASH.js gzip | 1.44 kB | 1.44 kB | ✓ |
Overall change | 71.3 kB | 71.3 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | sannajammeh/next.js add/emit-decorator-metadata | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | sannajammeh/next.js add/emit-decorator-metadata | Change | |
---|---|---|---|
_app-HASH.js gzip | 1.35 kB | 1.35 kB | ✓ |
_error-HASH.js gzip | 180 B | 180 B | ✓ |
amp-HASH.js gzip | 305 B | 305 B | ✓ |
css-HASH.js gzip | 321 B | 321 B | ✓ |
dynamic-HASH.js gzip | 2.36 kB | 2.36 kB | ✓ |
head-HASH.js gzip | 342 B | 342 B | ✓ |
hooks-HASH.js gzip | 911 B | 911 B | ✓ |
image-HASH.js gzip | 4.97 kB | 4.97 kB | ✓ |
index-HASH.js gzip | 256 B | 256 B | ✓ |
link-HASH.js gzip | 2.21 kB | 2.21 kB | ✓ |
routerDirect..HASH.js gzip | 314 B | 314 B | ✓ |
script-HASH.js gzip | 375 B | 375 B | ✓ |
withRouter-HASH.js gzip | 309 B | 309 B | ✓ |
85e02e95b279..7e3.css gzip | 107 B | 107 B | ✓ |
Overall change | 14.3 kB | 14.3 kB | ✓ |
Client Build Manifests
vercel/next.js canary | sannajammeh/next.js add/emit-decorator-metadata | Change | |
---|---|---|---|
_buildManifest.js gzip | 459 B | 459 B | ✓ |
Overall change | 459 B | 459 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | sannajammeh/next.js add/emit-decorator-metadata | Change | |
---|---|---|---|
index.html gzip | 533 B | 533 B | ✓ |
link.html gzip | 546 B | 546 B | ✓ |
withRouter.html gzip | 528 B | 528 B | ✓ |
Overall change | 1.61 kB | 1.61 kB | ✓ |
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.
Thanks for the PR!
fixes vercel#32913 Adds support for decorator metadata in SWC when enabled through ts/jsconfig. ## Bug - [ ] Related issues linked using `fixes #number` - [ ] Integration tests added - [ ] Errors have helpful link attached, see `contributing.md` ## Feature - [x] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR. - [x] Related issues linked using `fixes #number` - [x] Integration tests added - [ ] Documentation added - [ ] Telemetry added. In case of a feature if it's used or not. - [ ] Errors have helpful link attached, see `contributing.md` ## Documentation / Examples - [ ] Make sure the linting passes by running `yarn lint` Co-authored-by: JJ Kasper <22380829+ijjk@users.noreply.github.com>
fixes #32913
Adds support for decorator metadata in SWC when enabled through ts/jsconfig.
Bug
fixes #number
contributing.md
Feature
fixes #number
contributing.md
Documentation / Examples
yarn lint