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

Add support for transpile packages installed via npm #3319

Closed
wants to merge 4 commits into from
Closed

Add support for transpile packages installed via npm #3319

wants to merge 4 commits into from

Conversation

giuseppeg
Copy link
Contributor

@giuseppeg giuseppeg commented Nov 22, 2017

Fixes #706

Emits the transpiled modules in .next/dist/node_modules so that it works for SSR'd pages.
@arunoda / @nkzawa in case you like this solution I'll finish the PR.
If you really want I can try to make symlinks work too.

@arunoda
Copy link
Contributor

arunoda commented Nov 23, 2017

@giuseppeg This looks like a good approach.
I like it.

I'd like to see a test case as well.
We can simply publish a NPM package and use it.

@@ -206,6 +218,10 @@ export default async function createCompiler (dir, { buildId, dev = false, quiet
loader: 'emit-file-loader',
include: [dir, nextPagesDir],
exclude (str) {
if (shouldTranspileModule(str)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This works for sure.
For the SSR version, we may need to rewrite the require('my-module') pointing to this emitted file.

And if that requires a ES6+ module, how we are going to handle it?

Copy link
Contributor Author

@giuseppeg giuseppeg Nov 23, 2017

Choose a reason for hiding this comment

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

This works for SSR since node will look for .next/dist/node_modules first and pick up the transpiled version in it.

And if that requires a ES6+ module, how we are going to handle it?

During my testing my-modules was depending on another es6 module (in the same package) and both were transpiled and emitted to .next/dist/node_modules/my-modules.

Later today or tomorrow (when I have time basically) I will add those to the PR together with the changes to the readme.

As I said this solution still doesn't work with linked packages though - I will try to tackle that at the end and see if it is easy/doable to fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

@giuseppeg

This works for SSR since node will look for .next/dist/node_modules first and pick up the transpiled version in it.

That's pretty interesting. Nice work.

Looking forward to take this in.

Copy link
Contributor Author

@giuseppeg giuseppeg Nov 26, 2017

Choose a reason for hiding this comment

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

@arunoda edit: in order for this to work we need to copy the package.json over too. Do you have any idea how to do so? Or would you rather rewrite the imports/requires?

@rauchg
Copy link
Member

rauchg commented Nov 23, 2017

Do we want to support some sort of signal in the module config that it definitely won't have to be transpiled?

For example, if you require a module that has a devDependency on next, or something like that. Or { engines: { next } } or next: { }, or presence of next.config.js.

@arunoda
Copy link
Contributor

arunoda commented Nov 23, 2017

@rauchg @giuseppeg transpiring nested modules is tricky.
In that case, I'd like to see use explicit use of it.
Here's what I meant:

We look for config.transpileModules to get a list of modules we should transpile.
By default we don't transpile nested modules in those modules.
If the package.json of those modules has transpileModules field we use that to transpile nested modules.

Then this is clear.

@giuseppeg
Copy link
Contributor Author

giuseppeg commented Nov 26, 2017

@arunoda added an example.

By default we don't transpile nested modules in those modules.

Currently when imported, nested modules are transpiled and this is problematic.
We would need to rewrite the regexp to exclude sub dependencies like:

new RegExp(`${pattern.source}((?!node_modules).)*$`, pattern.flags)

Need to escape the original pattern probably.

If the package.json of those modules has transpileModules field we use that to transpile nested modules.

This is tricky, I wouldn't do that but rather have people add a regexp for it.

@arunoda
Copy link
Contributor

arunoda commented Dec 5, 2017

@giuseppeg

I was trying this today. But I'm having errors.
See:

screen shot 2017-12-04 at 5 16 05 pm
screen shot 2017-12-04 at 5 16 39 pm

I think the SSR version is not using the transpiled version.

Here's how I use this:

  • Run yarn link && next build inside a terminal in the main repo
  • Go to app directory of the example run yarn
  • Then run yarn link next
  • Then start the app with npm run dev

@giuseppeg
Copy link
Contributor Author

@arunoda yeah see my comment #3319 (comment)

If you copy the package.json of each module by hand to .next/dist/node_modules/component-a|b then it works.

@arunoda
Copy link
Contributor

arunoda commented Dec 5, 2017

@giuseppeg ah ha.
That's won't be impossible.
I'll try.

@ScottPolhemus
Copy link
Contributor

@giuseppeg @arunoda Thanks for your work on this! I've been able to work around the underlying issue in my own projects by customizing the exclude rules for each loader via next.config.js, but agree that a built-in configuration option would make the experience much better.

I took a slightly different approach in my own PR (#3470). There I've exposed a way to change the underlying exclude regex used by Next when creating the webpack loader configs. Curious what y'all think of that approach vs. the whitelist option used in this PR.

@giuseppeg
Copy link
Contributor Author

giuseppeg commented Dec 18, 2017

@ScottPolhemus I think that your solution suffers from the same issues as the one from this PR. Is SSR working for you? Also I personally think that with your solution one would easily end up having a ton of code transpiled or emitted since it is easy to involuntarily write regexps that are too permissive – whitelisting most of the times is better than blacklisting.

That said I might be missing something and be (happily) wrong.

@ScottPolhemus
Copy link
Contributor

ScottPolhemus commented Dec 18, 2017

Actually @giuseppeg, unless I'm missing something I don't think the error that @arunoda ran into above is related to SSR after all.

It looks like your example page (transpile-node-modules/app/pages/index.js) is attempting to import Logo from component-a, but there is no default export from component-a for you to import. (component-a includes a component in a subdirectory which has Logo imported as a dependency from component-b.) If I replace the first import on the index page with the following (which I'm assuming is close to what you intended), Next is able to compile the page:

import Logo from 'component-a/lib/zeit'

The actual error message was pretty misleading, but with this change your example runs fine for me.

I don't feel too strongly about using a whitelist vs. exposing the underlying exclude regex. I feel that this approach (the whitelist) obscures the actual webpack configuration from the user a little bit more (in the sense that it creates additional logic within Next for "magically" generating the appropriate loader rules, rather than expecting the user to actually know how webpack deals with things like the loaders' exclude options) which can be a good thing or a bad thing depending on your philosophy.

@giuseppeg
Copy link
Contributor Author

Yup it works if you require the actual module. The problem though is that many people will rely on the main field of package.json I think and get this error. If we also emit the package.json we are all good :) I will finish this branch during the xmas holidays.

I feel that this approach (the whitelist) obscures the actual webpack configuration from the user

This is great, better expose an API so you can always change the underlying implementation. Otherwise you are locked to that solution and can't make a change without breaking stuff.

@giuseppeg
Copy link
Contributor Author

By the way if you think that it could be useful we could discuss about an option to use the regexps for blacklisting i.e. make it configurable to support both.


<p><details>
<summary><b>Examples</b></summary>
<ul><li><a href="./examples/with-es6-npm-module-pages">Transpile NPM modules</a></li></ul>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: fix this link

Copy link
Contributor Author

@giuseppeg giuseppeg left a comment

Choose a reason for hiding this comment

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

@arunoda in my last commit you can see a way to copy the package.json files over. I am sure that there are edge cases where this might not work. I pushed this pof just to see what you/@rauchg think about it.

}

const packageJsonToCopy = {}
const copiedPackageJson = {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if this is persisted across builds (when rebuilding)

@@ -28,6 +28,13 @@ module.exports = function (content, sourceMap) {
callback(null, code, map)
}

if (query.copyPackageJson) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should/could be a separate loader

function getPackageJsonPath (filePath) {
let filePathDirname = dirname(filePath)
if (filePathDirname === dir) {
return join(dir, 'package.json')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need to check whether this file exists or probably return undefined in this case.

@maxs15
Copy link

maxs15 commented Jan 3, 2018

Is this PR functional @giuseppeg ? or still some work to be fully functional ?

@giuseppeg
Copy link
Contributor Author

@maxs15 functional? yep. Done? Not sure, it needs to be reviewed, polished and test for a few edge cases. Feel free to try it out locally.

@maxs15
Copy link

maxs15 commented Jan 4, 2018

Ok awesome work @giuseppeg, waiting for that feature for a long time !
I'll try it out

@maxs15
Copy link

maxs15 commented Jan 14, 2018

@giuseppeg do you think there is any way of being able to transpile a file outside of node_modules ?
It looks like the exclude function is only called when the files are the include property right ?
Do you think exposing a property to customize the include property could work ?

Thanks

@maxs15
Copy link

maxs15 commented Jan 14, 2018

It works but the issue that I have now is that the SSR doesn't require the compiled file in the .next directory.
Any idea which loader handle the case of requiring the file in .next if it exist ?

@jamesgorrie
Copy link

do you think there is any way of being able to transpile a file outside of node_modules ?

This would be great if it could do that, as things like Yarn workspaces should then be easier to work.
I am going to start trying that here: https://github.com/jamesgorrie/next.js/blob/with-yarn-workspaces/examples/with-yarn-workspaces

timneutkens added a commit that referenced this pull request Jan 30, 2018
* Speed up next build

* Document webpack config

* Speed up next build

* Remove comment

* Add comment

* Clean up rules

* Add comments

* Run in parallel

* Push plugins seperately

* Create a new chunk for react

* Don’t uglify react since it’s already uglified. Move react to commons in development

* Use the minified version directly

* Re-add globpattern

* Move loaders into a separate variable

* Add comment linking to Dan’s explanation

* Remove dot

* Add universal webpack

* Initial dev support

* Fix linting

* Add changes from Arunoda's work

* Made next dev works.
But super slow and no HMR support.

* Fix client side hot reload

* Server side hmr

* Only in dev

* Add on-demand-entries client + hot-middleware

* Add .babelrc support

* Speed up on demand entries by running in parallel

* Serve static generated files

* Add missing config in dev

* Add sass support

* Add support for .map

* Add cssloader config and fix .jsx support

* Rename

* use same defaults as css-loader. Fix linting

* Add NoEmitErrorsPlugin

* Add clientBootstrap

* Use webpackhotmiddleware on the multi compiler

* alpha.3

* Use babel 16.2.x

* Fix reloading after error

* Remove comment

* Release 5.0.0-univeral-alpha.1

* Remove check for React 16

* Release 5.0.0-universal-alpha.2

* React hot loader v4

* Use our static file rendering machanism to serve pages.
This should work well since the file path for a page is predictable.

* Release 5.0.0-universal-alpha.3

* Remove optional loaders

* Release 5.0.0-universal-alpha.4

* Remove clientBootstrap

* Remove renderScript

* Make sure pages bundles are served correctly

* Remove unused import

* Revert to using the same code as canary

* Fix hot loader

* Release 5.0.0-universal-alpha.5

* Check if externals dir exist before applying config

* Add typescript support

* Add support for transpiling certain packages in node_modules

Thanks to @giuseppeg’s work in #3319

* Add BABEL_DISABLE_CACHE support

* Make sourcemaps in production opt-in

* Revert "Add support for transpiling certain packages in node_modules"

This reverts commit d4b1d9b.

In favor of a better api around this.

* Support typescript through next.config.js

* Remove comments

* Bring back commons.js calculation

* Remove unused dependencies

* Move base.config.js to webpack.js

* Make sure to only invalidate webpackDevMiddleware one after other.

* Allow babel-loder caching by default.

* Add comment about preact support

* Bring back buildir replace

* Remove obsolete plugin

* Remove build replace, speed up build

* Resolve page entries like pages/day/index.js to pages/day.js

* Add componentDidCatch back

* Compile to bundles

* Use config.distDir everywhere

* Make sure the file is an array

* Remove console.log

* Apply optimization to uglifyjs

* Add comment pointing to source

* Create entries the same way in dev and production

* Remove unused and broken pagesGlobPattern

* day/index.js is automatically turned into day.js at build time

* Remove poweredByHeader option

* Load pages with the correct path.

* Release 5.0.0-universal-alpha.6

* Make sure react-dom/server can be overwritten by module-alias

* Only add react-hot-loader babel plugin in dev

* Release 5.0.0-universal-alpha.7

* Revert tests

* Release 5.0.0-universal-alpha.10

* Make sure next/head is working properly.

* Add wepack alias for 'next' back.

* Make sure overriding className in next/head works

* Alias react too

* Add missing r

* Fragment fallback has to wrap the children

* Use min.js

* Remove css.js

* Remove wallaby.js

* Release 5.0.0-universal-alpha.11

* Resolve relative to workdir instead of next

* Make sure we touch the right file

* Resolve next modules

* Remove dotjsx removal plugins since we use webpack on the server

* Revert "Resolve relative to workdir instead of next"

This reverts commit a13f3e4.

* Externalize any locally loaded module lives outside of app dir.

* Remove server aliases

* Check node_modules reliably

* Add symlink to next for tests

* Make sure dynamic imports work locally.
This is why we need it: https://github.com/webpack/webpack/blob/b545b519b2024e3f8be3041385bd326bf5d24449/lib/MainTemplate.js#L68
We need to have the finally clause in the above in __webpack_require__.
webpack output option strictModuleExceptionHandling does that.

* dynmaic -> dynamic

* Remove webpack-node-externals

* Make sure dynamic imports support SSR.

* Remove css support in favor of next-css

* Make sure we load path from `/` since it’s included in the path matching

* Catch when ensurepage couldn’t be fulfilled for `.js.map`

* Register require cache flusher for both client and server

* Add comment explaining this is to facilitate hot reloading

* Only load module when needed

* Remove unused modules

* Release 5.0.0-universal-alpha.12

* Only log the `found babel` message once

* Make sure ondemand entries working correctly.
Now we are just using a single instance of OnDemandEntryHandler.

* Better sourcemaps

* Release 5.0.0-universal-alpha.13

* Lock uglify version to 1.1.6

* Release 5.0.0-universal-alpha.14

* Fix a typo.

* Introduce multi-zones support for mircofrontends

* Add section on css
timneutkens added a commit that referenced this pull request Jan 30, 2018
* Speed up next build

* Document webpack config

* Speed up next build

* Remove comment

* Add comment

* Clean up rules

* Add comments

* Run in parallel

* Push plugins seperately

* Create a new chunk for react

* Don’t uglify react since it’s already uglified. Move react to commons in development

* Use the minified version directly

* Re-add globpattern

* Move loaders into a separate variable

* Add comment linking to Dan’s explanation

* Remove dot

* Add universal webpack

* Initial dev support

* Fix linting

* Add changes from Arunoda's work

* Made next dev works.
But super slow and no HMR support.

* Fix client side hot reload

* Server side hmr

* Only in dev

* Add on-demand-entries client + hot-middleware

* Add .babelrc support

* Speed up on demand entries by running in parallel

* Serve static generated files

* Add missing config in dev

* Add sass support

* Add support for .map

* Add cssloader config and fix .jsx support

* Rename

* use same defaults as css-loader. Fix linting

* Add NoEmitErrorsPlugin

* Add clientBootstrap

* Use webpackhotmiddleware on the multi compiler

* alpha.3

* Use babel 16.2.x

* Fix reloading after error

* Remove comment

* Release 5.0.0-univeral-alpha.1

* Remove check for React 16

* Release 5.0.0-universal-alpha.2

* React hot loader v4

* Use our static file rendering machanism to serve pages.
This should work well since the file path for a page is predictable.

* Release 5.0.0-universal-alpha.3

* Remove optional loaders

* Release 5.0.0-universal-alpha.4

* Remove clientBootstrap

* Remove renderScript

* Make sure pages bundles are served correctly

* Remove unused import

* Revert to using the same code as canary

* Fix hot loader

* Release 5.0.0-universal-alpha.5

* Check if externals dir exist before applying config

* Add typescript support

* Add support for transpiling certain packages in node_modules

Thanks to @giuseppeg’s work in #3319

* Add BABEL_DISABLE_CACHE support

* Make sourcemaps in production opt-in

* Revert "Add support for transpiling certain packages in node_modules"

This reverts commit d4b1d9b.

In favor of a better api around this.

* Support typescript through next.config.js

* Remove comments

* Bring back commons.js calculation

* Remove unused dependencies

* Move base.config.js to webpack.js

* Make sure to only invalidate webpackDevMiddleware one after other.

* Allow babel-loder caching by default.

* Add comment about preact support

* Bring back buildir replace

* Remove obsolete plugin

* Remove build replace, speed up build

* Resolve page entries like pages/day/index.js to pages/day.js

* Add componentDidCatch back

* Compile to bundles

* Use config.distDir everywhere

* Make sure the file is an array

* Remove console.log

* Apply optimization to uglifyjs

* Add comment pointing to source

* Create entries the same way in dev and production

* Remove unused and broken pagesGlobPattern

* day/index.js is automatically turned into day.js at build time

* Remove poweredByHeader option

* Load pages with the correct path.

* Release 5.0.0-universal-alpha.6

* Make sure react-dom/server can be overwritten by module-alias

* Only add react-hot-loader babel plugin in dev

* Release 5.0.0-universal-alpha.7

* Revert tests

* Release 5.0.0-universal-alpha.10

* Make sure next/head is working properly.

* Add wepack alias for 'next' back.

* Make sure overriding className in next/head works

* Alias react too

* Add missing r

* Fragment fallback has to wrap the children

* Use min.js

* Remove css.js

* Remove wallaby.js

* Release 5.0.0-universal-alpha.11

* Resolve relative to workdir instead of next

* Make sure we touch the right file

* Resolve next modules

* Remove dotjsx removal plugins since we use webpack on the server

* Revert "Resolve relative to workdir instead of next"

This reverts commit a13f3e4.

* Externalize any locally loaded module lives outside of app dir.

* Remove server aliases

* Check node_modules reliably

* Add symlink to next for tests

* Make sure dynamic imports work locally.
This is why we need it: https://github.com/webpack/webpack/blob/b545b519b2024e3f8be3041385bd326bf5d24449/lib/MainTemplate.js#L68
We need to have the finally clause in the above in __webpack_require__.
webpack output option strictModuleExceptionHandling does that.

* dynmaic -> dynamic

* Remove webpack-node-externals

* Make sure dynamic imports support SSR.

* Remove css support in favor of next-css

* Make sure we load path from `/` since it’s included in the path matching

* Catch when ensurepage couldn’t be fulfilled for `.js.map`

* Register require cache flusher for both client and server

* Add comment explaining this is to facilitate hot reloading

* Only load module when needed

* Remove unused modules

* Release 5.0.0-universal-alpha.12

* Only log the `found babel` message once

* Make sure ondemand entries working correctly.
Now we are just using a single instance of OnDemandEntryHandler.

* Better sourcemaps

* Release 5.0.0-universal-alpha.13

* Lock uglify version to 1.1.6

* Release 5.0.0-universal-alpha.14

* Fix a typo.

* Introduce multi-zones support for mircofrontends

* Add section on css
})
}

function getPackageJsonPath (filePath) {

Choose a reason for hiding this comment

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

Consider using https://www.npmjs.com/package/find-root instead

function getPackageJson (filePath) {
  const pkgRoot = findRoot(filePath)
  const packageJsonPath = path.resolve(pkgRoot, 'package.json')

  return packageJsonPath
}

@brettstack
Copy link

I'm currently refactoring into next.js. I had this working previously by deleting the exclude and specifying include on the loader to check if the package is either inside the project (default functionality) or if the package.json specified a module prop. The answers on this SO provide some good links on this proposed/de-facto standard prop https://stackoverflow.com/questions/42708484/what-is-the-module-package-json-field-for if you need more context on module.

This is how I'm transpiling node_modules by patching in next.config.js:

config.module.rules = config.module.rules.map(rule => {
  if (rule.use && rule.use.loader === 'babel-loader') {
    delete rule.exclude
    rule.include = (filePath) => {
      const shouldTranspile = pathIsInside(filePath, __dirname) || hasPkgModule(filePath)
      return shouldTranspile
    }
  }

  return rule
})

Rather, I'm trying to do this. I was doing the same thing in my webpack config previously and it was working fine. I confirmed the include rule is returning true for the module I'm trying to transpile but I'm getting SyntaxError: Unexpected token import (if you have any ideas, please let me know and save me some debugging time 🙏)

Maybe consider providing an option to transpile based on package.json.module also?

@giuseppeg
Copy link
Contributor Author

@brettstack with Next 5 and universal webpack probably we need to revisit this solution. I think that many things can go wrong when one tries to transpile external dependencies so I would understand if the folks at Zeit decided to discard this idea.

Anyway are you trying to patch Next 5's?

@timneutkens
Copy link
Member

You can use #3732

@pozylon
Copy link

pozylon commented Mar 28, 2018

Maybe helpful, this is how meteor tackles the problem in it's newest beta:
meteor/meteor#9559 (comment)

@giuseppeg
Copy link
Contributor Author

@timneutkens feel free to close this PR if you think that this solution won't work with the universal webpack rework

@timneutkens
Copy link
Member

👍

@lock lock bot locked as resolved and limited conversation to collaborators Mar 28, 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.

9 participants