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

List requirement to use main in package.json #8560

Closed
wants to merge 1 commit into from

Conversation

Vinnl
Copy link
Contributor

@Vinnl Vinnl commented Feb 27, 2020

I'm not 100% sure whether main is required to point to template.json, but when I did not include the key altogether, I ran into the following error:

Error: Cannot find module 'cra-template-[template-name]'
Require stack:
- /tmp/my-app/node_modules/react-scripts/scripts/init.js
- /tmp/my-app/[eval]
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:793:17)
    at Function.resolve (internal/modules/cjs/helpers.js:80:19)
    at module.exports (/tmp/my-app/node_modules/react-scripts/scripts/init.js:110:13)
    at [eval]:3:14
    at Script.runInThisContext (vm.js:116:20)
    at Object.runInThisContext (vm.js:306:38)
    at Object.<anonymous> ([eval]-wrapper:9:26)
    at Module._compile (internal/modules/cjs/loader.js:955:30)
    at evalScript (internal/process/execution.js:80:25)
    at internal/main/eval_string.js:23:3 {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/tmp/my-app/node_modules/react-scripts/scripts/init.js',
    '/tmp/my-app/[eval]'
  ]
}

@mrmckeb
Copy link
Contributor

mrmckeb commented Feb 29, 2020

Hi @Vinnl, from memory I hit this during testing and failed to document it. I think this is actually a requirement of the Node ecosystem - it is definitely not required by us.

@ianschmitz - what do you think, it's probably worth pointing out for people that don't use our templates as a reference.

@Vinnl
Copy link
Contributor Author

Vinnl commented Feb 29, 2020

Yeah that's what I presumed - that it didn't have to point to anything in particular, but that the key did have to be present. What the experience looked like for me was to first try and write a template using the documentation in its current shape, then getting the error message above, and then painstakingly comparing everything in my template to the official ones - took me quite a while to spot the difference. I could add a note that it's not required by CRA, but that it won't work if you don't add the key, but since that's not really actionable information, I'm not sure that it adds value.

@ianschmitz
Copy link
Contributor

Yeah it looks like it's because we're doing a require.resolve which then requires you to have main. There's probably a different way to do that, such as resolving the package.json of the package instead?

@mrmckeb
Copy link
Contributor

mrmckeb commented Mar 2, 2020

Good point @ianschmitz. @Vinnl, would you like to take a look at the require.resolve in init.js to see if there's a good alternative? Then we could eliminate this problem completely.

That code is responsible for getting the path of the template package.

@Vinnl
Copy link
Contributor Author

Vinnl commented Mar 2, 2020

@mrmckeb Unfortunately I'm not too familiar with require.resolve, although its documentation does not suggest it requiring a main, given that it intends to just find the module ID, which it should be able to get from just the name. Then again, it doesn't mention the second argument that is used by CRA ({ paths: [appPath] }) at all, so it might be incomplete.

I did find require.resolveWeak, which says it's

similar to require.resolve, but this won't pull the module into the bundle. It's what is considered a "weak" dependency.

So that does imply that require.resolve tries to load actual code (and needs to check main for that) - so perhaps simply replacing it with require.resolveWeak will eliminate this problem and continue to work?

@ianschmitz
Copy link
Contributor

Without trying that's my guess how it works under the hood. I've used something like this in past:

path.dirname(require.resolve("foo/package.json"))

@mrmckeb
Copy link
Contributor

mrmckeb commented Mar 9, 2020

Hi @Vinnl, did you want to take a look at this feature - as per the suggestion by @ianschmitz? I feel that would be a great addition and I'm happy to work with you on it.

If not, let me know and I can pick it up this week :)

@Vinnl
Copy link
Contributor Author

Vinnl commented Mar 9, 2020

Hi @mrmckeb, I've left a suggestion for a replacement above, and while I'd be happy to work on it, I cannot really justify the time investment of setting up a dev environment, learning about Webpack's internals and verifying that it does what I hope it does. So maybe just updating the docs is the way to go in the meantime.

@mrmckeb
Copy link
Contributor

mrmckeb commented Mar 9, 2020

@Vinnl, I understand open source can be time consuming, we are all volunteers on this project which is why we can be slow to respond.

I'll take a look at it over the week, as fixing the root cause would be a better solution.

@Vinnl
Copy link
Contributor Author

Vinnl commented Mar 9, 2020

Super! And to emphasise, I didn't expect you to fix it instead. I understand that it can be time-consuming and is a volunteer effort; unfortunately, the small docs fix was about all I can afford to contribute, but obviously it'd be great if someone can actually make it unnecessary.

@mrmckeb
Copy link
Contributor

mrmckeb commented Mar 9, 2020

No problem, I will make some time for this as I think others will hit the issue too.

@jebeck
Copy link

jebeck commented Mar 18, 2020

Just to chime in that I just hit this issue as well! I initially removed main pointing to src/index.js because that errored, tried an empty main after some of the discussion here, and ended on main pointing to template.json, which worked. Definitely was a confusing hurdle in trying to create a custom template, so an update to the docs would be much appreciated by future devs I think!

@Vinnl
Copy link
Contributor Author

Vinnl commented Mar 18, 2020

Might I suggest merging this in, and then later reverting it together with the fix that results in it no longer being necessary?

@mrmckeb
Copy link
Contributor

mrmckeb commented Mar 27, 2020

Hi all, I'll close this off in favour of #8734 - which we'll try to get out in the coming days.

@lock lock bot locked and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants