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

Make import stripping smarter to resolve #5019 #5114

Merged
merged 1 commit into from
Sep 15, 2017

Conversation

runspired
Copy link
Contributor

Previously we would only strip import declarations when all imports from a given module were previously filtered out by another plugin. This PR allows us to strip filtered imports from declarations without stripping the entire declaration if other imports are still needed.

@stefanpenner
Copy link
Member

stefanpenner commented Aug 8, 2017

LGTM, lets add a quick node-test smoke testing this transform, then we can ship-it.

@xomaczar
Copy link
Contributor

@stefanpenner @runspired can this be merge to resolve #5016 also in 2.15.0

@stefanpenner
Copy link
Member

@xomaczar unfortunately, not until it has tests. cc @runspired

@xomaczar
Copy link
Contributor

I don't mind pushing it forward, if u can clarify what kind of tests you are looking for. I assume something similar to the current e-d generators

@stefanpenner
Copy link
Member

@xomaczar a unit test, which uses babel directly, and configures it to use the above plugin, on various inputs confirming functionality works as expected.

@xomaczar
Copy link
Contributor

@stefanpenner thanks for the info @runspired thx for impl

@stefanpenner
Copy link
Member

@runspired thanks for the test! It does look like the new test is failing on CI, mind taking a look?

1) Unit: babel-plugin-remove-filtered-imports Strips :
     TypeError: unknown: Cannot read property 'name' of undefined
      at PluginPass.ImportDeclaration (lib/transforms/babel-plugin-remove-imports.js:36:58)
      at newFn (node_modules/babel-traverse/lib/visitors.js:276:21)
      at NodePath._call (node_modules/babel-traverse/lib/path/context.js:76:18)
      at NodePath.call (node_modules/babel-traverse/lib/path/context.js:48:17)
      at NodePath.visit (node_modules/babel-traverse/lib/path/context.js:105:12)
      at TraversalContext.visitQueue (node_modules/babel-traverse/lib/context.js:150:16)
      at TraversalContext.visitMultiple (node_modules/babel-traverse/lib/context.js:103:17)
      at TraversalContext.visit (node_modules/babel-traverse/lib/context.js:190:19)
      at Function.traverse.node (node_modules/babel-traverse/lib/index.js:114:17)
      at NodePath.visit (node_modules/babel-traverse/lib/path/context.js:115:19)

@runspired
Copy link
Contributor Author

@stefanpenner figured I'd push my working state in case others wanted to contribute. I figured out why the test fails though, babel output doesn't create the same AST for string transpile as it does for file transpile. Talked with @rwjblue and he pointed me to broccoli-test-helpers. Getting that working now.

@runspired
Copy link
Contributor Author

@stefanpenner r?

@runspired
Copy link
Contributor Author

The appveyor log suggests it was actually successful ...

let specifier = imports[i].imported;

if (!specifier) {
debugger;
Copy link
Member

Choose a reason for hiding this comment

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

i suspect this doesn't want to be here

@xomaczar
Copy link
Contributor

Merge?

@locks locks merged commit 575553f into emberjs:master Sep 15, 2017
@stefanpenner stefanpenner deleted the fix-import-stripping branch September 15, 2017 14:03
@stefanpenner
Copy link
Member

@locks are you able to backport and release as well?

@locks
Copy link
Contributor

locks commented Sep 15, 2017

@stefanpenner I'll only be able to do it on Sunday, if there's urgency take it away 💪 otherwise I'll tackle it.

@quaertym
Copy link

When is the expected release?

locks pushed a commit that referenced this pull request Sep 25, 2017
@locks locks mentioned this pull request Sep 25, 2017
@locks
Copy link
Contributor

locks commented Sep 25, 2017

Sorry, haven't had free weekends to handle this. I put up a backport PR at #5188.

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.

5 participants