Skip to content

Annotate frozen namespaces with #__PURE__ comment - #2044

Merged
lukastaegert merged 1 commit into
rollup:masterfrom
Andarist:pure-namespace
Mar 14, 2018
Merged

Annotate frozen namespaces with #__PURE__ comment#2044
lukastaegert merged 1 commit into
rollup:masterfrom
Andarist:pure-namespace

Conversation

@Andarist

Copy link
Copy Markdown
Member

fixes #2042

@kzc

kzc commented Mar 11, 2018

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

kzc commented Mar 11, 2018

Copy link
Copy Markdown
Contributor

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

@kzc

kzc commented Mar 11, 2018

Copy link
Copy Markdown
Contributor

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

kzc commented Mar 11, 2018

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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

Andarist commented Mar 13, 2018

Copy link
Copy Markdown
Member Author

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