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.

Sign up for GitHub

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

Jump to bottom

fixed babel configuration for #208 #240

Merged
merged 2 commits into from
Feb 12, 2023
Merged

fixed babel configuration for #208 #240

merged 2 commits into from
Feb 12, 2023

Conversation

vaukalak
Copy link
Contributor

@vaukalak vaukalak commented Jan 25, 2023 •
edited by ahangarha
Loading

In specific scenarios, you would need to transpile some of node_modules. This addition allows to specify node_modules to include via webpacker.yml => additional_paths configuration.

closes #208

@vaukalak vaukalak force-pushed the feature/SHP208 branch 2 times, most recently from 2e49f20 to d5b2d17 Compare January 25, 2023 18:31
@vaukalak vaukalak mentioned this pull request Jan 25, 2023
Closed
justin808
justin808 requested changes Jan 26, 2023
View reviewed changes
Copy link
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

  1. Create a new setting, like packages_to_transpile
  2. Don't overload additional_paths
  3. Needs documentation and changelog

package/rules/__tests__/__utils__/webpack.js Outdated
app_javascript,
createInMemoryFs,
createTestCompiler,
}
Copy link
Member

Choose a reason for hiding this comment

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

editor setup is incorrect

needs new line

Copy link
Contributor Author

@vaukalak vaukalak Jan 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@justin808 thanks fixed and created a new issue for eslint config: #242

package/rules/__tests__/babel.js Outdated
expect(tracked[ignored]).toBeUndefined();
});

test('process included node_modules', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

we should never transpile all of node_modules. Are you checking for only the additional paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@justin808 sure we do, and also we do have a test case for this. I'll work on a better verbiage.

package/rules/__tests__/babel.js Outdated
const [tracked, loader] = createTrackLoader();
const compiler = createTestCompiler(createWebpackConfig(included, loader));
await compiler.run();
expect(tracked[included]).toBeTruthy();
Copy link
Member

Choose a reason for hiding this comment

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

Need to check that that the files outside of node_modules/included and inside of node_modules are not transpiled, in at least one test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@justin808 we do have such test: https://github.com/shakacode/shakapacker/pull/240/files/d5b2d1744eaf17915aa36dd842e3d27ca414b317#diff-45330288787adaa7643887ecb63f58775bebe604da29c9789f0d5444a12a4ce7R44

package/rules/esbuild.js Show resolved Hide resolved
@vaukalak vaukalak force-pushed the feature/SHP208 branch 2 times, most recently from 18f87a9 to b0da1e9 Compare January 30, 2023 15:38
@vaukalak
Copy link
Contributor Author

  1. Create a new setting, like packages_to_transpile
  2. Don't overload additional_paths
  3. Needs documentation and changelog

@justin808 I've updated the changelog, but I'm not sure how adding additional property packages_to_transpile would be helpful. The code of webpack rules wouldn't change from what we have in this PR. We do override additional_paths only during tests. The only real difference would be in the configuration:

additional_paths: ['app/assets', 'node_modules/to_transpile']

vs

additional_paths: ['app/assets']
packages_to_transpile: ['node_modules/to_transpile']

So packages_to_transpile would become the same thing as additional_paths, however with node_modules support. Those properties will be merged anyway when creating the configuration for webpack. So, I don't know how it can be helpful, IMHO it will only add more confusion.

@vaukalak vaukalak requested a review from justin808 January 30, 2023 15:49
@justin808
Copy link
Member

Let's update the branch and merge it.

@justin808 justin808 merged commit fd23b30 into master Feb 12, 2023
@justin808 justin808 deleted the feature/SHP208 branch February 12, 2023 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justin808 justin808 Awaiting requested review from justin808

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

babel-loader rules ignores additional_paths that come from node_modules
2 participants
@vaukalak @justin808