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

Allow custom extensions #2699

Closed
wants to merge 8 commits into from
Closed

Allow custom extensions #2699

wants to merge 8 commits into from

Conversation

nelak
Copy link

@nelak nelak commented Aug 2, 2017

#2391 This PR adds support for custom extensions (.ts .tsx) based on the previous proposal: https://github.com/zeit/next.js/pull/2250/files#diff-0f2f34c098f5954f99483c9bd61e439dR51
Hope this also helps support .jsx

@D1no
Copy link

D1no commented Aug 6, 2017

I would love to see this getting merged!

@timneutkens
Copy link
Member

@nelak could you add tests for this? ❤️

@nelak
Copy link
Author

nelak commented Aug 6, 2017

@timneutkens I added an integration test for the custom extensions showcasing the usage of JSX and I added a new sample with-jsx to show how to use it.

@timneutkens
Copy link
Member

@nelak than you very much! Looks good 👌 The test will help when Arunoda has time to take a look at it 👍 ❤️

@nelak
Copy link
Author

nelak commented Aug 17, 2017

@timneutkens @arunoda Anything else I can do to help so this can be merged

@valorloff
Copy link

Please, add integration sample for the JSX extensions showcasing in context of Firebase-Function hosting specificity!

@ulrikstrid
Copy link

If there is anything blocking this I would love to help

@anaisbetts
Copy link

anaisbetts commented Sep 5, 2017

This is looking pretty good for us, except that static HTML export is broken:

> next build && next export

> Using "webpack" config function defined in next.config.js.
ts-loader: Using [email protected] and C:\Users\paulb\code\pulumi\pulumi-service\cmd\service-web\tsconfig.json
> Failed to build on C:\Users\paulb\AppData\Local\Temp\2d8827ed-3f74-4494-aaa3-a9fee5903a69
{ Error: C:\Users\paulb\code\pulumi\pulumi-service\cmd\service-web\out\_next\6c4331cd-421a-4bf7-80bc-ea271c311ce9\page\index.tsx
(2,18): error TS2339: Property '__NEXT_REGISTER_PAGE' does not exist on type 'Window'.
    at C:\Users\paulb\code\pulumi\pulumi-service\cmd\service-web\node_modules\@paulcbetts\next\dist\server\build\index.js:182:21
    at emitRecords.err (C:\Users\paulb\code\pulumi\pulumi-service\cmd\service-web\node_modules\webpack\lib\Compiler.js:269:13)
    at Compiler.emitRecords (C:\Users\paulb\code\pulumi\pulumi-service\cmd\service-web\node_modules\webpack\lib\Compiler.js:375:38)
    at emitAssets.err (C:\Users\paulb\code\pulumi\pulumi-service\cmd\service-web\node_modules\webpack\lib\Compiler.js:262:10)
    at applyPluginsAsyncSeries1.err (C:\Users\paulb\code\pulumi\pulumi-service\cmd\service-web\node_modules\webpack\lib\Compiler.js:368:12)
    at next (C:\Users\paulb\code\pulumi\pulumi-service\cmd\service-web\node_modules\tapable\lib\Tapable.js:218:11)
    at Compiler.compiler.plugin (C:\Users\paulb\code\pulumi\pulumi-service\cmd\service-web\node_modules\webpack\lib\performance\SizeLimitsPlugin.js:99:4)
    at Compiler.applyPluginsAsyncSeries1 (C:\Users\paulb\code\pulumi\pulumi-service\cmd\service-web\node_modules\tapable\lib\Tapable.js:222:13)
    at Compiler.afterEmit (C:\Users\paulb\code\pulumi\pulumi-service\cmd\service-web\node_modules\webpack\lib\Compiler.js:365:9)
    at require.forEach.err (C:\Users\paulb\code\pulumi\pulumi-service\cmd\service-web\node_modules\webpack\lib\Compiler.js:360:15)
  errors:
   [ 'C:\\Users\\paulb\\code\\pulumi\\pulumi-service\\cmd\\service-web\\out\\_next\\6c4331cd-421a-4bf7-80bc-ea271c311ce9\\page\\index.tsx\n\u001b[37m(\u001b[39m\u001b[36m2\u001b[39m,\u001b[36m18\u001b[39m): \u001b[31merror TS2339: Property \'__NEXT_REGISTER_PAGE\' does not exist on type \'Window\'.\u001b[39m',
     'C:\\Users\\paulb\\code\\pulumi\\pulumi-service\\cmd\\service-web\\out\\_next\\6c4331cd-421a-4bf7-80bc-ea271c311ce9\\page\\index.tsx\n\u001b[37m(\u001b[39m\u001b[36m4\u001b[39m,\u001b[36m1\u001b[39m): \u001b[31merror TS2304: Cannot find name \'webpackJsonp\'.\u001b[39m' ],

This file probably needs to be index.jsx maybe?

          window.__NEXT_REGISTER_PAGE('/', function() {
            var comp = module.exports =
webpackJsonp([2],[],[534]);
            return { page: comp.default }
          })

@srghma
Copy link

srghma commented Sep 5, 2017

also _document.tsx is ignored, reproduction

I wrote about that actually

@nelak
Copy link
Author

nelak commented Sep 6, 2017

@paulcbetts Can you provide me some context or sample and I'll try to take a look at it when I can

@nelak
Copy link
Author

nelak commented Sep 6, 2017

@paulcbetts I just gave this a try and at least for the with-typescript example you can get it to build and export properly the first time, excluding the out folder in the tsconfig.json will avoid it being recompiled on subsequent runs.

"exclude":[
        "node_modules",
        "out/**"
    ]

@nelak nelak changed the base branch from v3-beta to master September 6, 2017 03:38
@anaisbetts
Copy link

@nelak What tsconfig.json are you using? It could be that yours isn't strict enough maybe?

{
  "compilerOptions": {
    "target": "es2017",
    "module": "commonjs",
    "moduleResolution": "node",
    "declaration": true,
    "sourceMap": true,
    "stripInternal": true,
    "experimentalDecorators": true,
    "pretty": true,
    "noFallthroughCasesInSwitch": true,
    "noImplicitAny": true,
    "noImplicitReturns": true,
    "forceConsistentCasingInFileNames": true,
    "strictNullChecks": true,
    "jsx": "react",
    "lib": [ "dom", "es2017" ]
  }
}

Another option you could try is to disable compiler errors during export entirely, since build will be picking them up already

@nelak
Copy link
Author

nelak commented Sep 6, 2017

@paulcbetts I've updated the PR to include the sample with the export functionality, which should work.
My guess on the issue, it seems that ts-loader is picking up the out/ folder and trying to compile it causing the issue you are seeing. With the tsconfig.json change I was able to produce more than one export.
Can you try excluding the out/ folder from the in tsconfig.json configuration as shown below:

{
  "compilerOptions": {
    "target": "es2017",
    "module": "commonjs",
    "moduleResolution": "node",
    "declaration": true,
    "sourceMap": true,
    "stripInternal": true,
    "experimentalDecorators": true,
    "pretty": true,
    "noFallthroughCasesInSwitch": true,
    "noImplicitAny": true,
    "noImplicitReturns": true,
    "forceConsistentCasingInFileNames": true,
    "strictNullChecks": true,
    "jsx": "react",
    "lib": [ "dom", "es2017" ]
  },
  "exclude":[
        "node_modules",
        "out/**"
    ]
}

@D1no
Copy link

D1no commented Sep 16, 2017

Any ETA on this :/ ?

@lednhatkhanh
Copy link

This should be merged asap

@kulshekhar
Copy link

I need this for a project that I'm going to be using TypeScript for, and I'm willing to spend time on this if it helps. If there's anything I can do to help speed up the merging of this PR, let me know.

@srghma
Copy link

srghma commented Sep 21, 2017

just want to remind that _document.tsx is ignored

join(i, 'index.js'),
i + '.json',
join(i, 'index.json')
...getPagesPaths(config.pagesExtensions)
Copy link
Member

Choose a reason for hiding this comment

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

Why use a generator here 🤔

Copy link
Author

Choose a reason for hiding this comment

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

Because I wanted to leave open the possibility of handling multiple extensions.
Let say you want to support ts, tsx, jsx, etc... this avoids having to concat each of the combinations just having the generator yield all of the possibilities, since the generator is iterable you can concat it as shown above.


module.exports = {
// Add extensions
pagesExtensions: [

Choose a reason for hiding this comment

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

Would it be more convenient for users to be able to specify extensions as .${ext}?

@D1no
Copy link

D1no commented Oct 24, 2017

@nelak @valorloff @ulrikstrid @paulcbetts @BjornMelgaard @lednhatkhanh @kulshekhar
Since this PR is kind of dead, go checkout static-react — we're looking into bringing a very modular and reacty typescript implementation to life: react-static/react-static#8

Contributions are very welcome !

@kulshekhar
Copy link

@D1no just curious - is that project used in production by anyone at all? I mean, it doesn't have a single test (which makes the travis badge somewhat baffling)

@D1no
Copy link

D1no commented Oct 25, 2017

@kulshekhar yes, the project comes from an production environment. We do use it in an internal environment for a client and are very happy. And soon will lay over our own system on top of it. And of course by noozle.

Anyhow, the project is fairly young. But since it is build completely on top of well tested frameworks (React 16, React-Router, Webpack) with a very thin layer of stringing those together, without project proprietary implementations, E2E tests are something that should come in as soon the version settles. The leap forward here is really through React 16 hydration and React-Router v4 ability to create a AST of all routes used in the code.

I'd suggest just to try it out.

@joslarson
Copy link

@D1no why do you say this pull request is dead? I don't see any mention of that. What am I missing?

@chingyawhao
Copy link

chingyawhao commented Nov 21, 2017

Is this pull request still in consideration?
How can we help to have this merge asap?
There is this property pagesGlobPattern in the config of canary branch, isn't it in a way conflicting with pagesExtensions?

@JeremyJonas
Copy link

@nelak Any update on getting this merged in?

@nelak
Copy link
Author

nelak commented Nov 30, 2017

@JeremyJonas I once went through some bits of the code with @timneutkens so my guess is that this or some kind of variant will make it in at some point in the future.
Before it was possible to rebase and use this branch as needed but I can't confirm if it'll work since I haven't been keeping to date for a while now.

@voydz
Copy link

voydz commented Dec 20, 2017

@timneutkens Are there any further updates on this, you got for us?

@arunoda arunoda force-pushed the master branch 2 times, most recently from 647a42d to 33f8f28 Compare January 13, 2018 15:26
@renehauck
Copy link

any updates? 😿

@timneutkens
Copy link
Member

This is superseded by #3578

robdmoore added a commit to dddwa/dddperth-website that referenced this pull request Feb 28, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jan 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.