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

Annotate frozen namespaces with #__PURE__ comment #2044

Merged
merged 1 commit into from
Mar 14, 2018

Conversation

Andarist
Copy link
Member

fixes #2042

@kzc
Copy link
Contributor

kzc commented Mar 11, 2018

I'm trying to envision a real life use case. Will any of these tests produce a pure annotated Object.freeze call that will be dropped by uglify? Since all the references appear to be used, I suspect not.

@Andarist
Copy link
Member Author

This particular annotation is targeting consumers' of splitted libraries.

// library/index.js
import * as selectors from './chunk1.js'
console.log('selectors', selectors)
// library/selectors.js
export { selectorA, selectorB }  from './chunk1.js'
// library/chunk1.js
export const selectorA = () => {}
export const selectorB = () => {}

export const selectors = /*#__PURE__*/Object.freeze({
  selectorA,  
  selectorB,  
})
// ./consumer.js
import { selectorA } from 'library/selectors'
selectorA()

@kzc
Copy link
Contributor

kzc commented Mar 11, 2018

I see the use case now - deep linked named imports from chunked libraries.

@kzc
Copy link
Contributor

kzc commented Mar 11, 2018

It sounds like if we could get treeshaking to work for this use case then that would also solve the issue right?

As long as side effects are taken into consideration in a freeze of an object literal, sure:

const unused = Object.freeze({a: side_effect()});

@kzc
Copy link
Contributor

kzc commented Mar 11, 2018

Adding the pure annotation comment has one advantage - the rolled up chunked library will allow for webpack with scope hoisting to drop the unnecessary call. The ultimate consumer of the library may not be using Rollup.

@Andarist
Copy link
Member Author

the rolled up chunked library will allow for webpack with scope hoisting to drop the unnecessary call. The ultimate consumer of the library may not be using Rollup.

Exactly what I'm after with this PR :)

@lukastaegert
Copy link
Member

@guybedford Yes, I already have plans how to get this to tree-shake, just didn't get to it yet. Doesn't make sense IMO doing this without refactoring all of the global handling to create a general solution for globals the only side-effect of which would be to modify (or call) one of their arguments. In this specific situation, the proper logic would include the statement if the argument is an included variable (or of course if the variable it is assigned to is used).

Otherwise, code-splitting is not even necessary for this to be useful. You could also export a namespace import from an external dependency as an object. All users of uglify would then benefit from this change.

@Andarist Andarist force-pushed the pure-namespace branch 2 times, most recently from 86e65a9 to f191014 Compare March 13, 2018 08:35
@Andarist
Copy link
Member Author

Andarist commented Mar 13, 2018

I've rebased my branch against latest master, this could be shipped with @guybedford's namespace improvements PR (if you decide this one is ready)

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.

Annotate frozen namespaces with #__PURE__
4 participants