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

No need to specify list for entry node file #147

Merged
merged 2 commits into from
Oct 7, 2020
Merged

No need to specify list for entry node file #147

merged 2 commits into from
Oct 7, 2020

Conversation

poppinlp
Copy link
Contributor

No need to specify the languages list for entry node file for easier further maintain and reduce possible mismatch here.

@michaelwittig
Copy link
Owner

The entry-node.js was added in #64
Let's see if @ephys has time to comment on your change. I have no idea if this causes other issues

1 similar comment
@michaelwittig
Copy link
Owner

The entry-node.js was added in #64
Let's see if @ephys has time to comment on your change. I have no idea if this causes other issues

@michaelwittig
Copy link
Owner

I'm closing this since we haven't received feedback from the author of the code that is changed @ephys

@ephys
Copy link
Contributor

ephys commented Sep 29, 2020

Oh, hello
I'm sorry I don't think I saw the notification for the mention you did a year ago

The only drawback I would see is if you want the node entry point to be bundable (some people bundle their node code to reduce startup time)
If that's not a desired use case, this change should work fine

@michaelwittig
Copy link
Owner

@ephys do you think this will work in browser environments? I'm not familiar using npm in frontend projects...

@ephys
Copy link
Contributor

ephys commented Oct 7, 2020

@michaelwittig With this PR, this specific file would not work in browser environments anymore, but that shouldn't be an issue because entry-node is only loaded by node, index will be used by browsers (through bundlers) and is still compatible

@michaelwittig michaelwittig reopened this Oct 7, 2020
@michaelwittig michaelwittig merged commit 96d38e0 into michaelwittig:master Oct 7, 2020
@michaelwittig
Copy link
Owner

michaelwittig commented Oct 7, 2020

Thanks @ephys for helping out here! Much appreciated!

Thanks @poppinlp for contributing 👍 And sorry for the looooooong delay

@poppinlp poppinlp deleted the require_dir branch October 9, 2020 03:50
@Oupsla Oupsla mentioned this pull request Oct 27, 2020
michaelwittig added a commit that referenced this pull request Oct 27, 2020
@michaelwittig
Copy link
Owner

I had to revert this change because of #212

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants